From: Rajat Jain <rajatja@google.com>
To: Rajat Jain <rajatxjain@gmail.com>
Cc: "Krzysztof Wilczyński" <kw@linux.com>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Alan Stern" <stern@rowland.harvard.edu>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
linux-pci <linux-pci@vger.kernel.org>,
"open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:"
<linux-usb@vger.kernel.org>, "Bjorn Helgaas" <helgaas@kernel.org>,
"Jesse Barnes" <jsbarnes@google.com>,
"Dmitry Torokhov" <dtor@google.com>
Subject: Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
Date: Wed, 12 May 2021 15:27:38 -0700 [thread overview]
Message-ID: <CACK8Z6HuKqgYQZGJZGQGr5FC96naV+1yXZuwYTy5Ydb5=k40KA@mail.gmail.com> (raw)
In-Reply-To: <CAA93t1ohAFM1U2xTvbd1J1dUCaZwh6GYNGib_AM0J7+qHwSf1A@mail.gmail.com>
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@gmail.com> 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@linux.com> 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
next prev parent reply other threads:[~2021-05-12 22:38 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-24 2:16 [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Rajat Jain
2021-04-24 2:16 ` [PATCH v2 2/2] pci: Support "removable" attribute for PCI devices Rajat Jain
2021-04-26 9:17 ` Oliver Neukum
2021-04-26 11:49 ` Rafael J. Wysocki
2021-04-26 13:01 ` David Laight
2021-04-26 19:47 ` Rajat Jain
2021-04-27 11:59 ` Oliver Neukum
2021-04-27 12:59 ` David Laight
2021-04-28 6:56 ` Oliver Neukum
2021-04-28 12:21 ` Rafael J. Wysocki
2021-04-29 9:03 ` Oliver Neukum
2021-04-29 9:57 ` Rafael J. Wysocki
2021-04-29 16:59 ` Rajat Jain
2021-05-11 21:30 ` Bjorn Helgaas
2021-05-11 22:15 ` Rajat Jain
2021-05-11 23:02 ` Bjorn Helgaas
2021-05-12 0:02 ` Rajat Jain
2021-05-12 22:28 ` Rajat Jain
2021-04-25 14:58 ` [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core Alan Stern
2021-05-11 19:22 ` Bjorn Helgaas
2021-05-11 21:36 ` Rajat Jain
2021-05-12 1:00 ` Krzysztof Wilczyński
2021-05-12 1:20 ` Rajat Jain
2021-05-12 22:27 ` Rajat Jain [this message]
2021-05-12 23:32 ` Krzysztof Wilczyński
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACK8Z6HuKqgYQZGJZGQGr5FC96naV+1yXZuwYTy5Ydb5=k40KA@mail.gmail.com' \
--to=rajatja@google.com \
--cc=bhelgaas@google.com \
--cc=dtor@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=helgaas@kernel.org \
--cc=jsbarnes@google.com \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rajatxjain@gmail.com \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.