On Dec 26, 2023 / 18:34, Bjorn Helgaas wrote:
> On Mon, Dec 25, 2023 at 06:26:56PM +0900, Shin'ichiro Kawasaki wrote:
> > ...
>
> > +static int p2sb_valid_resource(struct resource *res)
> > +{
> > + return res->flags ? 0 : -ENOENT;
> > +}
>
> This got worse because it's *named* like a boolean, but the return
> value can't be used like a boolean, which makes callers really hard to
> read, e.g., this:
>
> if (p2sb_valid_resource(res))
> /* do something */
>
> does exactly the opposite of what the reader expects.
>
> I see that you want to use this -ENOENT return value in the callers:
>
> > +static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
> > +{
> > + ...
> > + return p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res);
> > +}
>
> > + * 0 on success or appropriate errno value on error.
> > + */
> > +int p2sb_bar(struct pci_bus *bus, unsigned int devfn, struct resource *mem)
> > +{
> > + ...
> > + ret = p2sb_valid_resource(&cache->res);
> > + if (ret)
> > + return ret;
>
> But I think these would be much clearer as something like this:
>
> static bool p2sb_valid_resource(struct resource *res)
> {
> if (res->flags)
> return true;
>
> return false;
> }
>
> static int p2sb_scan_and_cache(struct pci_bus *bus, unsigned int devfn)
> {
> ...
> if (!p2sb_valid_resource(&p2sb_resources[PCI_FUNC(devfn)].res))
> return -ENOENT;
>
> return 0;
> }
I have to admit that the function name meaning is opposite... When I followed
Andy's idea to make the function to return -ENOENT, I should have renamed the
function to not cause the confusion.
IMO, Bjorn's idea above is easier to read. Assuming Andy is ok with it, I will
make p2sb_valid_resource() return boolean and respin the patch.
