[PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[Ronny Hegewald]

The patch "libata: allow sata_sil24 to opt-out of tag ordered submission" 
(72dd299d5039a336493993dcc63413cf31d0e662) introduces a regression with the 
sata_sil24 driver.

The new flag ATA_FLAG_LOWTAG accidentially uses the same bit as 
SIL24_FLAG_PCIX_IRQ_WOC in the driver. This activates code for Silicon Image 
3132, which is only suppossed to run under 3124.

ATA_FLAG_LOWTAG is only used in sata_sil24 and is planned to be removed soon, 
so lets just use another bit for the flag in sata_sil24. 

Signed-off-by: Ronny Hegewald <Ronny.Hegewald@online.de>
Cc: stable@vger.kernel.org

--- linux-3.18.5/drivers/ata/sata_sil24.c.org
+++ linux-3.18.5/drivers/ata/sata_sil24.c
@@ -247,7 +247,7 @@
 	SIL24_COMMON_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
 				  ATA_FLAG_NCQ | ATA_FLAG_ACPI_SATA |
 				  ATA_FLAG_AN | ATA_FLAG_PMP | ATA_FLAG_LOWTAG,
-	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
+	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 25), /* IRQ loss errata on PCI-X */
 
 	IRQ_STAT_4PORTS		= 0xf,
 };

Re: [PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[Christoph Hellwig]

On Thu, Apr 09, 2015 at 08:09:08PM +0000, Ronny Hegewald wrote:
> The patch "libata: allow sata_sil24 to opt-out of tag ordered submission" 
> (72dd299d5039a336493993dcc63413cf31d0e662) introduces a regression with the 
> sata_sil24 driver.
> 
> The new flag ATA_FLAG_LOWTAG accidentially uses the same bit as 
> SIL24_FLAG_PCIX_IRQ_WOC in the driver. This activates code for Silicon Image 
> 3132, which is only suppossed to run under 3124.
> 
> ATA_FLAG_LOWTAG is only used in sata_sil24 and is planned to be removed soon, 
> so lets just use another bit for the flag in sata_sil24. 
> 
> Signed-off-by: Ronny Hegewald <Ronny.Hegewald@online.de>
> Cc: stable@vger.kernel.org
> 
> --- linux-3.18.5/drivers/ata/sata_sil24.c.org
> +++ linux-3.18.5/drivers/ata/sata_sil24.c
> @@ -247,7 +247,7 @@
>  	SIL24_COMMON_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
>  				  ATA_FLAG_NCQ | ATA_FLAG_ACPI_SATA |
>  				  ATA_FLAG_AN | ATA_FLAG_PMP | ATA_FLAG_LOWTAG,
> -	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
> +	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 25), /* IRQ loss errata on PCI-X */

And this will clash as soon as the next flag is added.  Please don't
abuse the common flag space for driver specific ones, and add a separate
flags field for driver specific flags.

Re: [PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[Sergei Shtylyov]

Hello.

On 04/10/2015 02:15 PM, Christoph Hellwig wrote:

>> The patch "libata: allow sata_sil24 to opt-out of tag ordered submission"
>> (72dd299d5039a336493993dcc63413cf31d0e662) introduces a regression with the
>> sata_sil24 driver.

>> The new flag ATA_FLAG_LOWTAG accidentially uses the same bit as
>> SIL24_FLAG_PCIX_IRQ_WOC in the driver. This activates code for Silicon Image
>> 3132, which is only suppossed to run under 3124.
>>
>> ATA_FLAG_LOWTAG is only used in sata_sil24 and is planned to be removed soon,
>> so lets just use another bit for the flag in sata_sil24.
>>
>> Signed-off-by: Ronny Hegewald <Ronny.Hegewald@online.de>
>> Cc: stable@vger.kernel.org
>>
>> --- linux-3.18.5/drivers/ata/sata_sil24.c.org
>> +++ linux-3.18.5/drivers/ata/sata_sil24.c
>> @@ -247,7 +247,7 @@
>>   	SIL24_COMMON_FLAGS	= ATA_FLAG_SATA | ATA_FLAG_PIO_DMA |
>>   				  ATA_FLAG_NCQ | ATA_FLAG_ACPI_SATA |
>>   				  ATA_FLAG_AN | ATA_FLAG_PMP | ATA_FLAG_LOWTAG,
>> -	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
>> +	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 25), /* IRQ loss errata on PCI-X */

    This one also collides with ATA_FLAG_SAS_HOST.

> And this will clash as soon as the next flag is added.  Please don't
> abuse the common flag space for driver specific ones, and add a separate
> flags field for driver specific flags.

    Actually, bits 24-31 are reserved for the low-level driver usage (see the 
comment below ATA_FLAG_*), so it's the new ATA_FLAG_LOWTAG and 
ATA_FLAG_SAS_HOST that have violated the convention and should be moved 
(there's plenty of lower bits due to the removal of some obsolete flags).

WBR, Sergei

Re: [PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[Tejun Heo]

On Fri, Apr 10, 2015 at 07:55:25PM +0300, Sergei Shtylyov wrote:
> >>-	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 24), /* IRQ loss errata on PCI-X */
> >>+	SIL24_FLAG_PCIX_IRQ_WOC	= (1 << 25), /* IRQ loss errata on PCI-X */
> 
>    This one also collides with ATA_FLAG_SAS_HOST.
> 
> >And this will clash as soon as the next flag is added.  Please don't
> >abuse the common flag space for driver specific ones, and add a separate
> >flags field for driver specific flags.
> 
>    Actually, bits 24-31 are reserved for the low-level driver usage (see the
> comment below ATA_FLAG_*), so it's the new ATA_FLAG_LOWTAG and
> ATA_FLAG_SAS_HOST that have violated the convention and should be moved
> (there's plenty of lower bits due to the removal of some obsolete flags).

Yeah, this something dumb I did way back.  Can we please add a
separate field for per-controller flags in port_info and all related
structs so that we don't do this sharing?

Thanks.

-- 
tejun

Re: [PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[Sergei Shtylyov]

Hello.

On 04/09/2015 11:09 PM, Ronny Hegewald wrote:

> The patch "libata: allow sata_sil24 to opt-out of tag ordered submission"
> (72dd299d5039a336493993dcc63413cf31d0e662) introduces a regression with the
> sata_sil24 driver.

> The new flag ATA_FLAG_LOWTAG accidentially uses the same bit as

    Accidentally.

> SIL24_FLAG_PCIX_IRQ_WOC in the driver. This activates code for Silicon Image
> 3132, which is only suppossed to run under 3124.

    Supposed.

> ATA_FLAG_LOWTAG is only used in sata_sil24 and is planned to be removed soon,
> so lets just use another bit for the flag in sata_sil24.

    Actually, ATA_FLAG_LOWTAG has been removed in the 'for-next' branch, so 
this patch is no longer needed. Moreover, the newly assigned flag clashes with 
the new ATA_FLAG_SATA_PORT.

> Signed-off-by: Ronny Hegewald <Ronny.Hegewald@online.de>

[...]

MBR, Sergei

Re: [PATCH] resolve collision of generic ATA_FLAG_LOWTAG and driver specific flag

[Ronny Hegewald]

On Friday 10 April 2015, you wrote
> Actually, ATA_FLAG_LOWTAG has been removed in the 'for-next' branch, so
> this patch is no longer needed. 

Its still needed for kernel <= 3.19, else the driver will malfunction for 
SI3132 based chipsets.

> Moreover, the newly assigned flag clashes with the new ATA_FLAG_SATA_PORT.

As far i can tell its not really a problem in practice for this driver, as 
they will not be used together. But its certainly better to keep the flags 
clash-free nonetheless. I will change it to bit 31 in the next version of the 
patch.