Received-SPF: pass (google.com: domain of linux-kernel+bounces-213243-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 13 Jun 2024 14:56:19 +0200
From: Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To: Damien Le Moal <dlemoal@kernel.org>
Cc: Niklas Cassel <cassel@kernel.org>, Werner Fischer <devlists@wefi.net>, 
	Daniel Drake <drake@endlessos.org>, Mika Westerberg <mika.westerberg@linux.intel.com>, 
	Jian-Hong Pan <jhp@endlessos.org>, Dieter Mummenschanz <dmummenschanz@web.de>, 
	Mario Limonciello <mario.limonciello@amd.com>, linux-ide@vger.kernel.org, regressions@lists.linux.dev, 
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/5] ata: ahci: a hotplug capable port is an external
 port
Message-ID: <d1b3642d-45a8-4550-ae04-af3897a33571@t-8ch.de>
References: <20240206211352.1664816-1-cassel@kernel.org>
 <20240206211352.1664816-3-cassel@kernel.org>
 <c0de8262-dc4b-4c22-9fac-33432e5bddd3@t-8ch.de>
 <63b12a50-7921-4f61-b41f-74e074c5ceb3@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <63b12a50-7921-4f61-b41f-74e074c5ceb3@kernel.org>

+Cc LKML for people to find it more easily.

On 2024-06-13 17:29:31+0000, Damien Le Moal wrote:
> 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) {

Thanks, looks good.

Tested-by: Thomas Weißschuh <linux@weissschuh.net>

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

Who takes care of 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