Re: [PATCH v3 06/13] peci: Add device detection

[Winiarska, Iwona]

On Tue, 2021-11-16 at 07:26 +0100, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
> On Mon, Nov 15, 2021 at 10:35:23PM +0000, Winiarska, Iwona wrote:
> > On Mon, 2021-11-15 at 19:49 +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Nov 15, 2021 at 07:25:45PM +0100, Iwona Winiarska wrote:
> > > > +void peci_device_destroy(struct peci_device *device)
> > > > +{
> > > > +       bool killed;
> > > > +
> > > > +       device_lock(&device->dev);
> > > > +       killed = kill_device(&device->dev);
> > > 
> > > Eeek, why call this?
> > > 
> > > > +       device_unlock(&device->dev);
> > > > +
> > > > +       if (!killed)
> > > > +               return;
> > > 
> > > What happened if something changed after you unlocked it?
> > 
> > We either killed it, or the other caller killed it.
> > 
> > > 
> > > Why is kill_device() required at all?  That's a very rare function to
> > > call, and one that only one "bus" calls today because it is very
> > > special (i.e. crazy and broken...)
> > 
> > It's used to avoid double-delete in case of races between peci_controller
> > unregister and "manually" removing the device using sysfs (pointed out by Dan
> > in
> > v2). We're calling peci_device_destroy() in both callsites.
> > Other way to solve it would be to just have a peci-specific lock, but
> > kill_device seemed to be well suited for the problem at hand.
> > Do you suggest to remove it and just go with the lock?
> 
> Yes please, remove it and use the lock.

Ack.

> 
> Also, why are you required to have a sysfs file that can remove the
> device?  Who wants that?

From the following patch:

"PECI devices may not be discoverable at the time when PECI controller is
being added (e.g. BMC can boot up when the Host system is still in S5).
Since we currently don't have the capabilities to figure out the Host
system state inside the PECI subsystem itself, we have to rely on
userspace to do it for us."

That's about rescan, but userspace might also want to remove the devices e.g.
when Host goes into S5.
It's also useful for development and debug purposes (and also allows us to have
a nice bit of symmetry with rescan).

Thanks
-Iwona

> 
> thanks,
> 
> greg k-h