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?
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
