Re: [PATCH v1 1/4] ACPI: PNP: Drop PNP0C01 and PNP0C02 from acpi_pnp_device_ids[]

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Dec 29, 2025 at 02:12:10PM +0100, Rafael J. Wysocki wrote:
> On Sun, Dec 28, 2025 at 8:11 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Dec 15, 2025 at 02:34:06PM +0100, Rafael J. Wysocki wrote:

...

> > > +     if (match_string(acpi_system_dev_ids, -1, acpi_device_hid(device)) >= 0) {
> >
> > Using -1 makes sense when we have no direct visibility of the mentioned array.
> > Here we have it visible and statically defined, hence the ARRAY_SIZE() is more
> > appropriate.
> 
> But as it stands it is more consistent with what happens elsewhere in this file.

It's not a big deal, especially if there is some consistency involved.

...

> > > +static void acpi_scan_claim_resources(struct acpi_device *adev)
> > > +{
> > > +     struct list_head resource_list = LIST_HEAD_INIT(resource_list);
> > > +     struct resource_entry *rentry;
> > > +     unsigned int count = 0;
> > > +     const char *regionid;
> >
> > > +     if (acpi_dev_get_resources(adev, &resource_list, NULL, NULL) <= 0)
> > > +             return;
> >
> > Strictly speaking the acpi_dev_free_resource_list() still needs to be called
> > on 0 return as it's called only for the error cases.
> 
> The return value of 0 means that the list is empty, so there's nothing to free.
> 
> > I think this is the first and the only time I see a combined comparison <= 0
> > for the acpi_dev_get_resources().
> 
> I don't see a problem here though.

There is no problem, the only minor thing is that the pattern I see seems only
here.

> > > +     regionid = kstrdup(dev_name(&adev->dev), GFP_KERNEL);
> > > +     if (!regionid)
> > > +             goto exit;
> > > +
> > > +     list_for_each_entry(rentry, &resource_list, node) {
> > > +             struct resource *res = rentry->res;
> > > +             struct resource *r;
> > > +
> > > +             /* Skip disabled and invalid resources. */
> > > +             if ((res->flags & IORESOURCE_DISABLED) || res->end < res->start)
> > > +                     continue;
> >
> > > +             if (res->flags & IORESOURCE_IO) {
> >
> > We have resource_type() helper.
> 
> I had considered using it, but then I couldn't convince myself that it
> would matter, so I went for simpler code.
> 
> > And I believe the direct comparison in this case is better.
> 
> You seem to be arguing that it matters after all, so why does it matter?

In the light of the resent findings of the resource_size() use on zeroed data
it also might be that in some cases the opposite happens, i.e. the resource had
initially a more bits of type assigned while checked for the certain one(s).
In such case the resource_type() == IORESOURCE_foo will fail on those.

I assume, nevertheless, that the code in question is quite unlikely exposed to
such mistakes.

-- 
With Best Regards,
Andy Shevchenko