All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed

From: "Krzysztof Wilczyński" <kw@linux.com>
To: Rajat Jain <rajatja@google.com>
Cc: 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@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-usb@vger.kernel.org, helgaas@kernel.org,
	rajatxjain@gmail.com, jsbarnes@google.com, dtor@google.com
Subject: Re: [PATCH v2 1/2] driver core: Move the "removable" attribute from USB to core
Date: Wed, 12 May 2021 03:00:49 +0200	[thread overview]
Message-ID: <20210512010049.GA89346@rocinante.localdomain> (raw)
In-Reply-To: <20210424021631.1972022-1-rajatja@google.com>

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?

[...]
> +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?

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

Krzysztof

next prev parent reply	other threads:[~2021-05-12  1:00 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 [this message]
2021-05-12  1:20   ` Rajat Jain
2021-05-12 22:27     ` Rajat Jain
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=20210512010049.GA89346@rocinante.localdomain \
    --to=kw@linux.com \
    --cc=bhelgaas@google.com \
    --cc=dtor@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=helgaas@kernel.org \
    --cc=jsbarnes@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=rajatja@google.com \
    --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

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.