[REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Laura Abbott]

Hi,

We received a bugzilla report https://bugzilla.redhat.com/show_bug.cgi?id=1310682
of partitions being automounted unexpectedly. Testing showed that
8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable") was responsible.
This seems to be classified as 'breaking' userspace given that this behavior results
in partitions that were previously unmounted now being being mounted unwanted
automatically which is unwanted behavior for the user. Can we revert this patch or
give a fix to change this behavior?

Thanks,
Laura

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Manuel Lauss]

On Thu, Feb 25, 2016 at 6:12 PM, Laura Abbott <labbott@redhat.com> wrote:
> Hi,
>
> We received a bugzilla report
> https://bugzilla.redhat.com/show_bug.cgi?id=1310682
> of partitions being automounted unexpectedly. Testing showed that
> 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable") was
> responsible.
> This seems to be classified as 'breaking' userspace given that this behavior
> results
> in partitions that were previously unmounted now being being mounted
> unwanted
> automatically which is unwanted behavior for the user. Can we revert this
> patch or
> give a fix to change this behavior?

This patch doesn't cause esata disks to be automounted on my distro (gentoo),
it just let's them appear in KDE's removable device widget for me
to mount it at a klick's notice.
I'm not convinced this is a "breaking userspace" problem.

Manuel

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Manuel Lauss]

On Thu, Feb 25, 2016 at 6:12 PM, Laura Abbott <labbott@redhat.com> wrote:
> Hi,
>
> We received a bugzilla report
> https://bugzilla.redhat.com/show_bug.cgi?id=1310682
> of partitions being automounted unexpectedly. Testing showed that
> 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable") was
> responsible.
> This seems to be classified as 'breaking' userspace given that this behavior
> results
> in partitions that were previously unmounted now being being mounted
> unwanted
> automatically which is unwanted behavior for the user. Can we revert this
> patch or
> give a fix to change this behavior?

Do the affected users have sata ports set to hotplug capable? Because that's
what the patch does: it flags ports which have the hotplug capability enabled
as "removable", and I guess redhat just automounts anything that is flagged
as a removable disk (like an usb drive).

Manuel

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Manuel Lauss]

On Thu, Feb 25, 2016 at 6:12 PM, Laura Abbott <labbott@redhat.com> wrote:
> Hi,
>
> We received a bugzilla report
> https://bugzilla.redhat.com/show_bug.cgi?id=1310682
> of partitions being automounted unexpectedly. Testing showed that
> 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable") was
> responsible.
> This seems to be classified as 'breaking' userspace given that this behavior
> results
> in partitions that were previously unmounted now being being mounted
> unwanted
> automatically which is unwanted behavior for the user. Can we revert this
> patch or
> give a fix to change this behavior?

Can you test this?  I think the old HPCP bit should just be ignored,
although windows does use it, i.e. when I enable hotplug capability
in the bios on my desktop it shows the  "eject device" icon for a device
attached to the port on the motherboard, we probably can ignore it
on linux to not break all automounting distros.  On my laptop, where
I care about this, it still works as intended.

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 4029679..e029e3c 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1164,8 +1164,7 @@ static void ahci_port_init(struct device *dev,
struct ata_port *ap,

        /* mark esata ports */
        tmp = readl(port_mmio + PORT_CMD);
-       if ((tmp & PORT_CMD_HPCP) ||
-           ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS)))
+       if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
                ap->pflags |= ATA_PFLAG_EXTERNAL;
 }

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Laura Abbott]

On 02/25/2016 10:56 AM, Manuel Lauss wrote:
> On Thu, Feb 25, 2016 at 6:12 PM, Laura Abbott <labbott@redhat.com> wrote:
>> Hi,
>>
>> We received a bugzilla report
>> https://bugzilla.redhat.com/show_bug.cgi?id=1310682
>> of partitions being automounted unexpectedly. Testing showed that
>> 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable") was
>> responsible.
>> This seems to be classified as 'breaking' userspace given that this behavior
>> results
>> in partitions that were previously unmounted now being being mounted
>> unwanted
>> automatically which is unwanted behavior for the user. Can we revert this
>> patch or
>> give a fix to change this behavior?
>
> Do the affected users have sata ports set to hotplug capable? Because that's
> what the patch does: it flags ports which have the hotplug capability enabled
> as "removable", and I guess redhat just automounts anything that is flagged
> as a removable disk (like an usb drive).
>
> Manuel
>

The mounting comes from udisks. udisks will automount anything marked as removable
unless explicitly forbidden. You can argue whether or not this is good design but
it's still a change in behavior from what udisks was relying on to decide
whether or not to automount. lspci from the users hardware:

00:1f.2 SATA controller: Intel Corporation C600/X79 series chipset 6-Port SATA
AHCI Controller (rev 06)

I'll make a build with the patch you gave for testing.

Thanks,
Laura

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Tejun Heo]

Hello, Laura.

On Thu, Feb 25, 2016 at 11:37:09AM -0800, Laura Abbott wrote:
> The mounting comes from udisks. udisks will automount anything marked as removable
> unless explicitly forbidden. You can argue whether or not this is good design but
> it's still a change in behavior from what udisks was relying on to decide
> whether or not to automount. lspci from the users hardware:
> 
> 00:1f.2 SATA controller: Intel Corporation C600/X79 series chipset 6-Port SATA
> AHCI Controller (rev 06)

It is a behavior change but difficult to characterize as a breakage,
given that we do want userland to treat ports explicitly marked as
external to be treated as such.  That said, eSATA being mostly a
fringe thing, we can go "whatever" and revert it.

Let's hope it's a false positive that the patch can fix.

Thanks.

-- 
tejun

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Laura Abbott]

On 02/25/2016 11:09 AM, Manuel Lauss wrote:
> On Thu, Feb 25, 2016 at 6:12 PM, Laura Abbott <labbott@redhat.com> wrote:
>> Hi,
>>
>> We received a bugzilla report
>> https://bugzilla.redhat.com/show_bug.cgi?id=1310682
>> of partitions being automounted unexpectedly. Testing showed that
>> 8a3e33cf92c7 ("ata: ahci: find eSATA ports and flag them as removable") was
>> responsible.
>> This seems to be classified as 'breaking' userspace given that this behavior
>> results
>> in partitions that were previously unmounted now being being mounted
>> unwanted
>> automatically which is unwanted behavior for the user. Can we revert this
>> patch or
>> give a fix to change this behavior?
>
> Can you test this?  I think the old HPCP bit should just be ignored,
> although windows does use it, i.e. when I enable hotplug capability
> in the bios on my desktop it shows the  "eject device" icon for a device
> attached to the port on the motherboard, we probably can ignore it
> on linux to not break all automounting distros.  On my laptop, where
> I care about this, it still works as intended.
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 4029679..e029e3c 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1164,8 +1164,7 @@ static void ahci_port_init(struct device *dev,
> struct ata_port *ap,
>
>          /* mark esata ports */
>          tmp = readl(port_mmio + PORT_CMD);
> -       if ((tmp & PORT_CMD_HPCP) ||
> -           ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS)))
> +       if ((tmp & PORT_CMD_ESP) && (hpriv->cap & HOST_CAP_SXS))
>                  ap->pflags |= ATA_PFLAG_EXTERNAL;
>   }
>

At least one user reported that this fixed the issue and he said
you could add

Tested-by: Bob Schultz <bob@engineer.com>

Thanks,
Laura

Re: [REGRESSION] 8a3e33cf92c7 "ata: ahci: find eSATA ports and flag them as removable" changes userspace behavior

[Tejun Heo]

Hello,

On Thu, Feb 25, 2016 at 05:49:34PM -0800, Laura Abbott wrote:
> At least one user reported that this fixed the issue and he said
> you could add
> 
> Tested-by: Bob Schultz <bob@engineer.com>

Great, Manuel, can you please send a proper patch?

Thanks.

-- 
tejun