[v2,22/22] ata: ahci_xgene: Fix id array access in xgene_ahci_read_id()

ATA IDENTIFY command returns an array of le16 words. Accessing it as a
u16 array triggers the following sparse warning:

drivers/ata/ahci_xgene.c:262:33: warning: invalid assignment: &=
drivers/ata/ahci_xgene.c:262:33:    left side has type unsigned short
drivers/ata/ahci_xgene.c:262:33:    right side has type restricted __le16

Use a local variable to explicitly cast the id array to __le16 to avoid
this warning.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/ata/ahci_xgene.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On 1/4/22 11:58 AM, Damien Le Moal wrote:
> ATA IDENTIFY command returns an array of le16 words. Accessing it as a
> u16 array triggers the following sparse warning:
> 
> drivers/ata/ahci_xgene.c:262:33: warning: invalid assignment: &=
> drivers/ata/ahci_xgene.c:262:33:    left side has type unsigned short
> drivers/ata/ahci_xgene.c:262:33:    right side has type restricted __le16
> 
> Use a local variable to explicitly cast the id array to __le16 to avoid
> this warning.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/ata/ahci_xgene.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
> index 68ec7e9430b2..d5075d0f8cb1 100644
> --- a/drivers/ata/ahci_xgene.c
> +++ b/drivers/ata/ahci_xgene.c
> @@ -239,6 +239,7 @@ static bool xgene_ahci_is_memram_inited(struct xgene_ahci_context *ctx)
>  static unsigned int xgene_ahci_read_id(struct ata_device *dev,
>  				       struct ata_taskfile *tf, u16 *id)
>  {
> +	__le16 *__id = (__le16 *)id;
>  	u32 err_mask;
>  
>  	err_mask = ata_do_dev_read_id(dev, tf, id);
> @@ -259,7 +260,7 @@ static unsigned int xgene_ahci_read_id(struct ata_device *dev,
>  	 *
>  	 * Clear reserved bit 8 (DEVSLP bit) as we don't support DEVSLP
>  	 */
> -	id[ATA_ID_FEATURE_SUPP] &= cpu_to_le16(~(1 << 8));
> +	__id[ATA_ID_FEATURE_SUPP] &= cpu_to_le16(~(1 << 8));
>  
>  	return 0;
>  }
> 
Hmm. I would think that the 'id' argument is wrong; it really should be
'__le16 *', as it only gets converted later on with the call to
swap_buf_le16(id, ATA_ID_WORDS) in
drivers/ata/libata-core.c:ata_dev_read_id(). So when this function is
called the argument really _is_ __le16, only the declaration doesn't
tell us.

Maybe one should rather fix this?

Cheers,

Hannes

On 1/4/22 20:51, Hannes Reinecke wrote:
> On 1/4/22 11:58 AM, Damien Le Moal wrote:
>> ATA IDENTIFY command returns an array of le16 words. Accessing it as a
>> u16 array triggers the following sparse warning:
>>
>> drivers/ata/ahci_xgene.c:262:33: warning: invalid assignment: &=
>> drivers/ata/ahci_xgene.c:262:33:    left side has type unsigned short
>> drivers/ata/ahci_xgene.c:262:33:    right side has type restricted __le16
>>
>> Use a local variable to explicitly cast the id array to __le16 to avoid
>> this warning.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  drivers/ata/ahci_xgene.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/ahci_xgene.c b/drivers/ata/ahci_xgene.c
>> index 68ec7e9430b2..d5075d0f8cb1 100644
>> --- a/drivers/ata/ahci_xgene.c
>> +++ b/drivers/ata/ahci_xgene.c
>> @@ -239,6 +239,7 @@ static bool xgene_ahci_is_memram_inited(struct xgene_ahci_context *ctx)
>>  static unsigned int xgene_ahci_read_id(struct ata_device *dev,
>>  				       struct ata_taskfile *tf, u16 *id)
>>  {
>> +	__le16 *__id = (__le16 *)id;
>>  	u32 err_mask;
>>  
>>  	err_mask = ata_do_dev_read_id(dev, tf, id);
>> @@ -259,7 +260,7 @@ static unsigned int xgene_ahci_read_id(struct ata_device *dev,
>>  	 *
>>  	 * Clear reserved bit 8 (DEVSLP bit) as we don't support DEVSLP
>>  	 */
>> -	id[ATA_ID_FEATURE_SUPP] &= cpu_to_le16(~(1 << 8));
>> +	__id[ATA_ID_FEATURE_SUPP] &= cpu_to_le16(~(1 << 8));
>>  
>>  	return 0;
>>  }
>>
> Hmm. I would think that the 'id' argument is wrong; it really should be
> '__le16 *', as it only gets converted later on with the call to
> swap_buf_le16(id, ATA_ID_WORDS) in
> drivers/ata/libata-core.c:ata_dev_read_id(). So when this function is
> called the argument really _is_ __le16, only the declaration doesn't
> tell us.
> 
> Maybe one should rather fix this?

Good point. Sending V3 with this patch changed to fix read_id()
operation interface.

> 
> Cheers,
> 
> Hannes