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
