Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core

[Rajat Jain]

Posted a v3 of this patch here:
https://lore.kernel.org/patchwork/patch/1428133/

On Tue, May 11, 2021 at 6:21 PM Rajat Jain <rajatxjain@xxxxxxxxx> wrote:
>
> Hi Krzysztof,
>
> Thanks a lot for your comments. Please see inline.
>
> On Tue, May 11, 2021 at 6:00 PM Krzysztof Wilczyński <kw@xxxxxxxxx> wrote:
> >
> > Hi Rajat,
> >
> > I have few questions below, but to add in advance, I might be confusing
> > the role that "type->supports_removable" and "dev->removable" plays
> > here, and if so then I apologise.
> >
> > [...]
> > > @@ -2504,8 +2523,16 @@ static int device_add_attrs(struct device *dev)
> > >                       goto err_remove_dev_online;
> > >       }
> > >
> > > +     if (type && type->supports_removable) {
> > > +             error = device_create_file(dev, &dev_attr_removable);
> > > +             if (error)
> > > +                     goto err_remove_dev_waiting_for_supplier;
> > > +     }
> > > +
> > >       return 0;
> >
> > Would a check for "dev->removable == DEVICE_REMOVABLE" here be more
> > appropriate?
> >
> > Unless you wanted to add sysfs objects when the device hints that it has
> > a notion of being removable even though it might be set to "unknown" or
> > "fixed" (if that state is at all possible then), and in which case using
> > the dev_is_removable() helper would also not be an option since it does
> > a more complex check internally.
> >
> > Technically, you could always add this sysfs object (similarly to what
> > USB core did) as it would then show the correct state depending on
> > "dev->removable".
> >
> > Also, I suppose, it's not possible for a device to have
> > "supports_removable" set to true, but "removable" would be different
> > than "DEVICE_REMOVABLE", correct?
>
> No, that is not true.
>
> device_type->supports_removable=1 indicates that the bus / subsystem
> is capable of differentiating between removable and fixed devices.
> It's essentially describing a capability of the bus / subsystem. This
> flag needs to be true for a subsystem for any it's devices'
> dev->removable field to be considered meaningful.
>
> OTOH, the dev->removable => indicates the location of the device IF
> device_type->supports location is true. Yes, it can be fixed /
> removable / unknown (whatever the bus decides) if the
> device_type->supports_location is true.
>
> One of my primary considerations was also that the existing UAPI for
> the USB's "removable" attribute shouldn't be changed. Currently, it
> exists for all USB devices, so I think the current code / check is OK.
>
> >
> > [...]
> > > +enum device_removable {
> > > +     DEVICE_REMOVABLE_UNKNOWN = 0,
> > > +     DEVICE_REMOVABLE,
> > > +     DEVICE_FIXED,
> > > +};
> >
> > I know this was moved from the USB core, but I personally find it
> > a little bit awkward to read, would something like that be acceptable?
> >
> > enum device_removable {
> >         DEVICE_STATE_UNKNOWN = 0,
> >         DEVICE_STATE_REMOVABLE,
> >         DEVICE_STATE_FIXED,
> > };
> >
> > The addition of state to the name follows the removable_show() function
> > where the local variable is called "state", and I think it makes sense
> > to call this as such.  What do you think?
>
> I think I made a mistake by using the "state" as the local variable
> there. I will change it to "location". I'm happy to change the enums
> above to DEVICE_LOCATION_REMOVABLE* etc if there is a wider consensus
> on this. IMHO, the current shorter one also looks OK.
>
> >
> > > +static inline bool dev_is_removable(struct device *dev)
> > > +{
> > > +     return dev && dev->type && dev->type->supports_removable
> > > +         && dev->removable == DEVICE_REMOVABLE;
> > > +}
> >
> > Similarly to my question about - would a simple check to see if
> > "dev->removable" is set to "DEVICE_REMOVABLE" here be enough?
>
> No, as I mentioned above, the dev->removable field should be
> considered meaningful only if device_type->supports_location is true.
> So the check for supports_removable is needed here.
>
> Please feel free to send me more thoughts.
>
> Thanks & Best Regards,
>
> Rajat
>
>
> >
> > Krzysztof