On Thu, May 13, 2021 at 9:40 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, May 13, 2021 at 09:26:26AM -0700, Rajat Jain wrote:
> > > > static ssize_t ltm_capable_show(struct device *dev,
> > > > struct device_attribute *attr, char *buf)
> > > > {
> > > > @@ -828,7 +805,6 @@ static struct attribute *dev_attrs[] = {
> > > > &dev_attr_avoid_reset_quirk.attr,
> > > > &dev_attr_authorized.attr,
> > > > &dev_attr_remove.attr,
> > > > - &dev_attr_removable.attr,
> > > > &dev_attr_ltm_capable.attr,
> > > > #ifdef CONFIG_OF
> > > > &dev_attr_devspec.attr,
> > > > diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> > > > index 62368c4ed37a..ce18e84528cf 100644
> > > > --- a/drivers/usb/core/usb.c
> > > > +++ b/drivers/usb/core/usb.c
> > > > @@ -569,6 +569,7 @@ struct device_type usb_device_type = {
> > > > #ifdef CONFIG_PM
> > > > .pm = &usb_device_pm_ops,
> > > > #endif
> > > > + .supports_removable = true,
> > > > };
> > > >
> > > >
> > > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > > index 38a2071cf776..7e87ab048307 100644
> > > > --- a/include/linux/device.h
> > > > +++ b/include/linux/device.h
> > > > @@ -93,6 +93,8 @@ struct device_type {
> > > > void (*release)(struct device *dev);
> > > >
> > > > const struct dev_pm_ops *pm;
> > > > +
> > > > + bool supports_removable:1; /* subsystem can classify removable/fixed */
> > >
> > > Why isn't this a bus type? Shouldn't it go there and not in the device
> > > type?
> >
> > Please see below.
> >
> > >
> > > > };
> > > >
> > > > /* interface for exporting device attributes */
> > > > @@ -350,6 +352,19 @@ enum dl_dev_state {
> > > > DL_DEV_UNBINDING,
> > > > };
> > > >
> > > > +/**
> > > > + * enum device_removable - Whether the device is removable. The criteria for a
> > > > + * device to be classified as removable is determined by its subsystem or bus.
> > > > + * @DEVICE_REMOVABLE_UNKNOWN: Device location is Unknown (default).
> > > > + * @DEVICE_REMOVABLE: Device is removable by the user.
> > > > + * @DEVICE_FIXED: Device is not removable by the user.
> > > > + */
> > > > +enum device_removable {
> > > > + DEVICE_REMOVABLE_UNKNOWN = 0,
> > > > + DEVICE_REMOVABLE,
> > > > + DEVICE_FIXED,
> > > > +};
> > > > +
> > > > /**
> > > > * struct dev_links_info - Device data related to device links.
> > > > * @suppliers: List of links to supplier devices.
> > > > @@ -431,6 +446,9 @@ struct dev_links_info {
> > > > * device (i.e. the bus driver that discovered the device).
> > > > * @iommu_group: IOMMU group the device belongs to.
> > > > * @iommu: Per device generic IOMMU runtime data
> > > > + * @removable: Whether the device can be removed from the system. This
> > > > + * should be set by the subsystem / bus driver that discovered
> > > > + * the device.
> > > > *
> > > > * @offline_disabled: If set, the device is permanently online.
> > > > * @offline: Set after successful invocation of bus type's .offline().
> > > > @@ -544,6 +562,8 @@ struct device {
> > > > struct iommu_group *iommu_group;
> > > > struct dev_iommu *iommu;
> > > >
> > > > + enum device_removable removable;
> > > > +
> > > > bool offline_disabled:1;
> > > > bool offline:1;
> > > > bool of_node_reused:1;
> > > > @@ -782,6 +802,18 @@ static inline bool dev_has_sync_state(struct device *dev)
> > > > return false;
> > > > }
> > > >
> > > > +static inline void dev_set_removable(struct device *dev,
> > > > + enum device_removable removable)
> > > > +{
> > > > + dev->removable = removable;
> > > > +}
> > > > +
> > > > +static inline bool dev_is_removable(struct device *dev)
> > > > +{
> > > > + return dev && dev->type && dev->type->supports_removable
> > > > + && dev->removable == DEVICE_REMOVABLE;
> > >
> > > Again, shouldn't this be a bus type, and not a device type?
> > >
> > > Where are you going to have devices of different types on a bus that do,
> > > or do not, allow this attribute?
> >
> > USB. Presently, both the usb_device_type and usb_if_device_type sit on
> > the usb_bus_type but "removable" only applies to usb_device_type (the
> > attribute shows up only under usb_devices and not under
> > usb_interfaces).
>
> Ah. Messy.
>
> Then should this be a per-device value and don't worry about the type at
> all? You have a field in the device already, why not make it:
>
> enum device_removable {
> DEVICE_REMOVABLE_NOT_SUPPORTED = 0, /* must be 0 */
> DEVICE_REMOVABLE_UNKNOWN,
> DEVICE_REMOVABLE,
> DEVICE_FIXED,
> };
>
> And then only show the file if a device does NOT have the value set to
> DEVICE_REMOVABLE_NOT_SUPPORTED?
>
> If a bus/subsystem/whatever wants the device to have a sysfs file and
> value there, then it needs to set it to anything other than 0 and it
> will be created?
Ack, will do.
Thanks,
Rajat
>
> That gets you out of the device bus/type mess and it is localized to
> only the device itself, allowing for any device to do this? Should make
> the logic everywhere a bit simpler as well.
>
> thanks,
>
> greg k-h
