* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 17:33 [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices Niklas Cassel
@ 2024-06-13 17:35 ` Mario Limonciello
2024-06-13 17:40 ` Thomas Weißschuh
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Mario Limonciello @ 2024-06-13 17:35 UTC (permalink / raw)
To: Niklas Cassel, Damien Le Moal, Mika Westerberg
Cc: Manuel Lauss, Thomas Weißschuh, linux-ide
On 6/13/2024 12:33, Niklas Cassel wrote:
> From: Damien Le Moal <dlemoal@kernel.org>
>
> The SCSI Removable Media Bit (RMB) should only be set for removable media,
> where the device stays and the media changes, e.g. CD-ROM or floppy.
>
> The ATA removable media device bit is currently only defined in the CFA
> (CFast) specification, to indicate that the device can have its media
> removed (while the device stays).
>
> Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
> removable") introduced a change to set the RMB bit if the port has either
> the eSATA bit or the hot-plug capable bit set. The reasoning was that the
> author wanted his eSATA ports to get treated like a USB stick.
>
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
>
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
>
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
>
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
>
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
>
> Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
> external/removable") then changed so that the RMB bit is only set for the
> eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
> reports of SATA devices were being automounted by udisks. However,
> treating eSATA and hot-plug capable ports differently is not correct.
>
> From the AHCI 1.3.1 spec:
> Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
> signal and power connectors are externally accessible via a joint signal
> and power connector for blindmate device hot plug.
>
> So a hot-plug capable port is an external port, just like commit
> 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> claims.
>
> In order to not violate the SPC specification, modify the SCSI INQUIRY
> data to only set the RMB bit if the ATA device can have its media removed.
>
> This fixes a reported problem where GNOME/udisks was automounting devices
> connected to hot-plug capable ports.
>
> Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
> Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [cassel: wrote commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> drivers/ata/libata-scsi.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..e231ad22f88a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
> 2
> };
>
> - /* set scsi removable (RMB) bit per ata bit, or if the
> - * AHCI port says it's external (Hotplug-capable, eSATA).
> + /*
> + * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> + * device bit (which is only defined in the CFA specification) is set.
> */
> - if (ata_id_removable(args->id) ||
> - (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
> + if (ata_id_removable(args->id))
> hdr[1] |= (1 << 7);
>
> if (args->dev->class == ATA_DEV_ZAC) {
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 17:33 [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices Niklas Cassel
2024-06-13 17:35 ` Mario Limonciello
@ 2024-06-13 17:40 ` Thomas Weißschuh
2024-06-13 17:44 ` Niklas Cassel
2024-06-13 19:00 ` Sergei Shtylyov
3 siblings, 0 replies; 8+ messages in thread
From: Thomas Weißschuh @ 2024-06-13 17:40 UTC (permalink / raw)
To: Niklas Cassel
Cc: Damien Le Moal, Mika Westerberg, Mario Limonciello, Manuel Lauss,
linux-ide
On 2024-06-13 19:33:53+0000, Niklas Cassel wrote:
> From: Damien Le Moal <dlemoal@kernel.org>
>
> The SCSI Removable Media Bit (RMB) should only be set for removable media,
> where the device stays and the media changes, e.g. CD-ROM or floppy.
>
> The ATA removable media device bit is currently only defined in the CFA
> (CFast) specification, to indicate that the device can have its media
> removed (while the device stays).
>
> Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
> removable") introduced a change to set the RMB bit if the port has either
> the eSATA bit or the hot-plug capable bit set. The reasoning was that the
> author wanted his eSATA ports to get treated like a USB stick.
>
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
>
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
>
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
>
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
>
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
>
> Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
> external/removable") then changed so that the RMB bit is only set for the
> eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
> reports of SATA devices were being automounted by udisks. However,
> treating eSATA and hot-plug capable ports differently is not correct.
>
> From the AHCI 1.3.1 spec:
> Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
> signal and power connectors are externally accessible via a joint signal
> and power connector for blindmate device hot plug.
>
> So a hot-plug capable port is an external port, just like commit
> 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> claims.
>
> In order to not violate the SPC specification, modify the SCSI INQUIRY
> data to only set the RMB bit if the ATA device can have its media removed.
>
> This fixes a reported problem where GNOME/udisks was automounting devices
> connected to hot-plug capable ports.
>
> Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
> Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [cassel: wrote commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
While I would prefer a simple revert at this point in the release cycle:
Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
This patch should also have a "Cc: stable@vger.kernel.org".
> ---
> drivers/ata/libata-scsi.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..e231ad22f88a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
> 2
> };
>
> - /* set scsi removable (RMB) bit per ata bit, or if the
> - * AHCI port says it's external (Hotplug-capable, eSATA).
> + /*
> + * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> + * device bit (which is only defined in the CFA specification) is set.
> */
> - if (ata_id_removable(args->id) ||
> - (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
> + if (ata_id_removable(args->id))
> hdr[1] |= (1 << 7);
>
> if (args->dev->class == ATA_DEV_ZAC) {
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 17:33 [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices Niklas Cassel
2024-06-13 17:35 ` Mario Limonciello
2024-06-13 17:40 ` Thomas Weißschuh
@ 2024-06-13 17:44 ` Niklas Cassel
2024-06-13 17:49 ` Thomas Weißschuh
2024-06-13 19:00 ` Sergei Shtylyov
3 siblings, 1 reply; 8+ messages in thread
From: Niklas Cassel @ 2024-06-13 17:44 UTC (permalink / raw)
To: Damien Le Moal, Mika Westerberg, Mario Limonciello
Cc: Manuel Lauss, Thomas Weißschuh, linux-ide
On Thu, Jun 13, 2024 at 07:33:53PM +0200, Niklas Cassel wrote:
>
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
>
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
>
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
>
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
>
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
Since SPC-6 does make it quite clear that USB Memory Stick device servers
should not have the RMB bit set, Thomas, may I ask what udisks is using to
automount USB sticks?
Since USB sticks that follow SPC-6 clearly cannot have RMB bit set,
which means that SCSI core will set removable:
(the equivalent of:)
/sys/devices/pci0000:00/0000:00:04.0/ata3/host2/target2:0:0/2:0:0:0/block/sda/removable
to 0.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 17:44 ` Niklas Cassel
@ 2024-06-13 17:49 ` Thomas Weißschuh
2024-06-13 18:00 ` Niklas Cassel
0 siblings, 1 reply; 8+ messages in thread
From: Thomas Weißschuh @ 2024-06-13 17:49 UTC (permalink / raw)
To: Niklas Cassel
Cc: Damien Le Moal, Mika Westerberg, Mario Limonciello, Manuel Lauss,
linux-ide
On 2024-06-13 19:44:22+0000, Niklas Cassel wrote:
> On Thu, Jun 13, 2024 at 07:33:53PM +0200, Niklas Cassel wrote:
> >
> > This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> > Expectations" which has since been integrated to SPC, which states that:
> >
> > """
> > Reports have been received that some USB Memory Stick device servers set
> > the removable medium (RMB) bit to one. The rub comes when the medium is
> > actually removed, because... The device server is removed concurrently
> > with the medium removal. If there is no device server, then there is no
> > device server that is waiting to have removable medium inserted.
> >
> > Sufficient numbers of SCSI analysts see such a device:
> > - not as a device that supports removable medium;
> > but
> > - as a removable, hot pluggable device.
> > """
> >
> > The definition of the RMB bit in the SPC specification has since been
> > clarified to match this.
> >
> > Thus, a USB stick should not have the RMB bit set (and neither shall an
> > eSATA nor a hot-plug capable port).
>
> Since SPC-6 does make it quite clear that USB Memory Stick device servers
> should not have the RMB bit set, Thomas, may I ask what udisks is using to
> automount USB sticks?
As also mentioned at [0]:
/* Provide easy access to _only_ the following devices
*
* - anything connected via known local buses (e.g. USB or Firewire, MMC or MemoryStick)
* - any device with removable media
*
* Be careful when extending this list as we don't want to automount
* the world when (inadvertently) connecting to a SAN.
*/
From [1]
> Since USB sticks that follow SPC-6 clearly cannot have RMB bit set,
> which means that SCSI core will set removable:
> (the equivalent of:)
> /sys/devices/pci0000:00/0000:00:04.0/ata3/host2/target2:0:0/2:0:0:0/block/sda/removable
> to 0.
(I am not a udisks person, but we have the same problem in lsblk
regarding "RM" and "HOTPLUG" attributes)
[0] https://lore.kernel.org/all/6d5e7f17-6760-4128-a5d5-22ae2a87dadf@t-8ch.de/
[1] https://github.com/storaged-project/udisks/blob/8821a7808880ea37cdb299647c38f3a5ceb3d72a/src/udiskslinuxblock.c#L860
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 17:49 ` Thomas Weißschuh
@ 2024-06-13 18:00 ` Niklas Cassel
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Cassel @ 2024-06-13 18:00 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Damien Le Moal, Mika Westerberg, Mario Limonciello, Manuel Lauss,
linux-ide
On Thu, Jun 13, 2024 at 07:49:42PM +0200, Thomas Weißschuh wrote:
> On 2024-06-13 19:44:22+0000, Niklas Cassel wrote:
> > On Thu, Jun 13, 2024 at 07:33:53PM +0200, Niklas Cassel wrote:
> > >
> > > This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> > > Expectations" which has since been integrated to SPC, which states that:
> > >
> > > """
> > > Reports have been received that some USB Memory Stick device servers set
> > > the removable medium (RMB) bit to one. The rub comes when the medium is
> > > actually removed, because... The device server is removed concurrently
> > > with the medium removal. If there is no device server, then there is no
> > > device server that is waiting to have removable medium inserted.
> > >
> > > Sufficient numbers of SCSI analysts see such a device:
> > > - not as a device that supports removable medium;
> > > but
> > > - as a removable, hot pluggable device.
> > > """
> > >
> > > The definition of the RMB bit in the SPC specification has since been
> > > clarified to match this.
> > >
> > > Thus, a USB stick should not have the RMB bit set (and neither shall an
> > > eSATA nor a hot-plug capable port).
> >
> > Since SPC-6 does make it quite clear that USB Memory Stick device servers
> > should not have the RMB bit set, Thomas, may I ask what udisks is using to
> > automount USB sticks?
>
> As also mentioned at [0]:
>
> /* Provide easy access to _only_ the following devices
> *
> * - anything connected via known local buses (e.g. USB or Firewire, MMC or MemoryStick)
> * - any device with removable media
> *
> * Be careful when extending this list as we don't want to automount
> * the world when (inadvertently) connecting to a SAN.
> */
>
> From [1]
> [1] https://github.com/storaged-project/udisks/blob/8821a7808880ea37cdb299647c38f3a5ceb3d72a/src/udiskslinuxblock.c#L860
Unfortunately I can't find the definition of
udisks_drive_get_media_removable().
But looking at the name, I'm assuming that it checks if the _media_ is
removable, i.e. CD-ROM and floppy, etc., and not if the device is
removable/hot-pluggable.
So I think that the patch in $subject is the way to go.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 17:33 [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices Niklas Cassel
` (2 preceding siblings ...)
2024-06-13 17:44 ` Niklas Cassel
@ 2024-06-13 19:00 ` Sergei Shtylyov
2024-06-14 12:03 ` Niklas Cassel
3 siblings, 1 reply; 8+ messages in thread
From: Sergei Shtylyov @ 2024-06-13 19:00 UTC (permalink / raw)
To: Niklas Cassel, Damien Le Moal, Mika Westerberg, Mario Limonciello
Cc: Manuel Lauss, Thomas Weißschuh, linux-ide
On 6/13/24 8:33 PM, Niklas Cassel wrote:
> From: Damien Le Moal <dlemoal@kernel.org>
>
> The SCSI Removable Media Bit (RMB) should only be set for removable media,
> where the device stays and the media changes, e.g. CD-ROM or floppy.
>
> The ATA removable media device bit is currently only defined in the CFA
> (CFast) specification, to indicate that the device can have its media
> removed (while the device stays).
>
> Commit 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as
> removable") introduced a change to set the RMB bit if the port has either
> the eSATA bit or the hot-plug capable bit set. The reasoning was that the
> author wanted his eSATA ports to get treated like a USB stick.
>
> This is however wrong. See "20-082r23SPC-6: Removable Medium Bit
> Expectations" which has since been integrated to SPC, which states that:
>
> """
> Reports have been received that some USB Memory Stick device servers set
> the removable medium (RMB) bit to one. The rub comes when the medium is
> actually removed, because... The device server is removed concurrently
> with the medium removal. If there is no device server, then there is no
> device server that is waiting to have removable medium inserted.
>
> Sufficient numbers of SCSI analysts see such a device:
> - not as a device that supports removable medium;
> but
> - as a removable, hot pluggable device.
> """
>
> The definition of the RMB bit in the SPC specification has since been
> clarified to match this.
>
> Thus, a USB stick should not have the RMB bit set (and neither shall an
> eSATA nor a hot-plug capable port).
>
> Commit dc8b4afc4a04 ("ata: ahci: don't mark HotPlugCapable Ports as
> external/removable") then changed so that the RMB bit is only set for the
> eSATA bit (and not for the hot-plug capable bit), because of a lot of bug
> reports of SATA devices were being automounted by udisks. However,
> treating eSATA and hot-plug capable ports differently is not correct.
>
> From the AHCI 1.3.1 spec:
> Hot Plug Capable Port (HPCP): When set to '1', indicates that this port's
> signal and power connectors are externally accessible via a joint signal
> and power connector for blindmate device hot plug.
>
> So a hot-plug capable port is an external port, just like commit
> 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> claims.
>
> In order to not violate the SPC specification, modify the SCSI INQUIRY
> data to only set the RMB bit if the ATA device can have its media removed.
>
> This fixes a reported problem where GNOME/udisks was automounting devices
> connected to hot-plug capable ports.
>
> Fixes: 45b96d65ec68 ("ata: ahci: a hotplug capable port is an external port")
> Tested-by: Thomas Weißschuh <linux@weissschuh.net>
> Reported-by: Thomas Weißschuh <linux@weissschuh.net>
> Closes: https://lore.kernel.org/linux-ide/c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de/
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> [cassel: wrote commit message]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> drivers/ata/libata-scsi.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index cdf29b178ddc..e231ad22f88a 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
> 2
> };
>
> - /* set scsi removable (RMB) bit per ata bit, or if the
> - * AHCI port says it's external (Hotplug-capable, eSATA).
> + /*
> + * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> + * device bit (which is only defined in the CFA specification) is set.
It used to be defined since ATA-1; I think it was only obsoleted by ATA-8 ACS...
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] ata: libata-scsi: Set the RMB bit only for removable media devices
2024-06-13 19:00 ` Sergei Shtylyov
@ 2024-06-14 12:03 ` Niklas Cassel
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Cassel @ 2024-06-14 12:03 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Damien Le Moal, Mika Westerberg, Mario Limonciello, Manuel Lauss,
Thomas Weißschuh, linux-ide
On Thu, Jun 13, 2024 at 10:00:30PM +0300, Sergei Shtylyov wrote:
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index cdf29b178ddc..e231ad22f88a 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1831,11 +1831,11 @@ static unsigned int ata_scsiop_inq_std(struct ata_scsi_args *args, u8 *rbuf)
> > 2
> > };
> >
> > - /* set scsi removable (RMB) bit per ata bit, or if the
> > - * AHCI port says it's external (Hotplug-capable, eSATA).
> > + /*
> > + * Set the SCSI Removable Media Bit (RMB) if the ATA removable media
> > + * device bit (which is only defined in the CFA specification) is set.
>
> It used to be defined since ATA-1; I think it was only obsoleted by ATA-8 ACS...
Looking at the t13 achives, it was obsoleted by:
e06116r0: Obsolete removable media related feature sets
in 2006, ATA-8 ACS (i.e. ACS-1).
I will update the comment accordingly and send out a v2.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 8+ messages in thread