Re: [PATCH v3] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux