Re: [PATCH v1 1/1] ACPI: scan: Use unique number for instance_no

[Andy Shevchenko]

On Fri, Mar 19, 2021 at 06:00:38PM +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 12, 2021 at 5:02 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
> >
> > Current mechanism of incrementing and decrementing plain integer
> > to get a next free instance_no when creating an ACPI device is fragile.
> >
> > In case of hot plug event or namespace removal of the device instances
> > with the low numbers the plain integer counter can't cover the gaps
> > and become desynchronized with real state of affairs. If during next
> > hot plug event or namespace injection the new instances of
> > the devices need to be instantiated, the counter may mistakenly point
> > to the existing instance_no and kernel will complain:
> > "sysfs: cannot create duplicate filename '/bus/acpi/devices/XXXX1234:02'"
> 
> This is a slightly convoluted way of stating that there is a bug in
> acpi_device_del().

Any suggestion how to massage the above?
But in the dry end, yes, decrementing is a bug.

> Yes, there is one, the instance_no decrementation is clearly incorrect.
> 
> > Replace plain integer approach by using IDA framework.
> 
> Also the general idea of using IDA for the instance numbering is a good one IMO.

...

> > -       unsigned int instance_no;
> > +       struct ida no;
> 
> struct ida instance_ida; ?

Will rename.

...

> > +       p = strrchr(dev_name(&device->dev), ':');
> > +       if (!p)
> > +               return -ENODATA;
> > +
> > +       error = kstrtoint(p + 1, 16, &result);
> > +       if (error)
> > +               return error;
> > +
> > +       return result;
> 
> I don't like the above at all.
> 
> I would just store the instance number in struct acpi_device_pnp (say).

TBH, I simply didn't know which struct to touch and left this one and I also
don't like it. Lemme see if acpi_device_pnp is good enough for that.

-- 
With Best Regards,
Andy Shevchenko