Re: [PATCH v2 2/5] ata: ahci: a hotplug capable port is an external port

[Damien Le Moal <dlemoal@kernel.org>]

On 6/13/24 15:34, Thomas Weißschuh wrote:
> Hi everbody,
> 
> On 2024-02-06 22:13:43+0000, Niklas Cassel wrote:
>> A hotplug capable port is an external port, so mark it as such.
>>
>> We even say this ourselves in libata-scsi.c:
>> /* set scsi removable (RMB) bit per ata bit, or if the
>>  * AHCI port says it's external (Hotplug-capable, eSATA).
>>  */
>>
>> This also matches the terminology used in AHCI 1.3.1
>> (the keyword to search for is "externally accessible").
>>
>> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
>> Signed-off-by: Niklas Cassel <cassel@kernel.org>
>> ---
>>  drivers/ata/ahci.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index aa58ce615e79..4d3ec6d15ad1 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1648,9 +1648,10 @@ static void ahci_mark_external_port(struct ata_port *ap)
>>  	void __iomem *port_mmio = ahci_port_base(ap);
>>  	u32 tmp;
>>  
>> -	/* mark esata ports */
>> +	/* mark external ports (hotplug-capable, eSATA) */
>>  	tmp = readl(port_mmio + PORT_CMD);
>> -	if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
>> +	if (((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS)) ||
>> +	    (tmp & PORT_CMD_HPCP))
>>  		ap->pflags |= ATA_PFLAG_EXTERNAL;
>>  }
> 
> This seems to introduce a userspace regression.
> 
> GNOME/udisks are now automounting internal disks, which they didn't before.
> See [0], [1], [2]
> 
> ATA_PFLAG_EXTERNAL is translated into GENHD_FL_REMOVABLE.
> (Through ata_scsiop_inq_std(), scsi_add_lun(), sd_probe())
> 
> But GENHD_FL_REMOVABLE is not meant for hotpluggable devices but for
> media-changable devices (See its description in include/linux/blkdev.h).
> 
> To indicate hotplug, dev_set_removable() is to be used.
> 
> (Both end up in "removable" sysfs attributes, but these have different
> semantics...)

This should take care of the issue.

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 37ded3875ea3..170ed47ef74a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1912,11 +1912,8 @@ 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).
-        */
-       if (ata_id_removable(args->id) ||
-           (args->dev->link->ap->pflags & ATA_PFLAG_EXTERNAL))
+       /* Set scsi removable (RMB) bit per ata bit. */
+       if (ata_id_removable(args->id))
                hdr[1] |= (1 << 7);

        if (args->dev->class == ATA_DEV_ZAC) {

BUT, need to check what SAT & SATA-IO have to say about this.

> 
> #regzbot introduced: 45b96d65ec68f625ad26ee16d2f556e29f715005
> 
> [0] https://bbs.archlinux.org/viewtopic.php?id=295958
> [1] https://github.com/storaged-project/udisks/issues/1282
> [2] https://github.com/util-linux/util-linux/issues/3088
> 

-- 
Damien Le Moal
Western Digital Research