Re: [PATCH v4] libata-scsi: better style in ata_msense_*()

[Tejun Heo]

On Wed, Jul 20, 2016 at 04:39:28AM +0800, tom.t...@gmail.com wrote:
> From: Tom Yan 
> 
> `changeable` is the "version" of mode page requested by the user.
> It will be less confusing/misleading if we do not check it
> "together" with the setting bits of the drive.
> 
> Not to mention that we currently have ata_mselect_*() implemented
> in a way that each of them will serve exclusively a particular bit
> on each page. The old style will hence make the condition look even
> more unnecessarily arcane if the ata_msense_*() is reflecting more
> than one bit.
> 
> Signed-off-by: Tom Yan 

Applied to libata/for-4.8.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] libata-scsi: better style in ata_msense_*()

[Tom Yan]

FWIW, I end up using `bool d_sense = (dev->flags &
ATA_DFLAG_D_SENSE)`, which was introduced to ata_scsi_set_sense() in
commit 06dbde5f3a44 ("libata: Implement control mode page to select
sense format"), in the polished ata_msense_control().

@Sergei, I kept the parentheses for the bit shifting/masking results
because it appears that the rest of the code in libata-scsi.c does
that as well. So I am not going to partially clean up that to avoid
making it look inconsistent.

On 20 July 2016 at 04:39,   wrote:
> From: Tom Yan 
>
> `changeable` is the "version" of mode page requested by the user.
> It will be less confusing/misleading if we do not check it
> "together" with the setting bits of the drive.
>
> Not to mention that we currently have ata_mselect_*() implemented
> in a way that each of them will serve exclusively a particular bit
> on each page. The old style will hence make the condition look even
> more unnecessarily arcane if the ata_msense_*() is reflecting more
> than one bit.
>
> Signed-off-by: Tom Yan 
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index bf4cb21..b00521b 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2424,10 +2424,12 @@ static void modecpy(u8 *dest, const u8 *src, int n, 
> bool changeable)
>  static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable)
>  {
> modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable);
> -   if (changeable || ata_id_wcache_enabled(id))
> -   buf[2] |= (1 << 2); /* write cache enable */
> -   if (!changeable && !ata_id_rahead_enabled(id))
> -   buf[12] |= (1 << 5);/* disable read ahead */
> +   if (changeable) {
> +   buf[2] |= (1 << 2); /* ata_mselect_caching() */
> +   } else {
> +   buf[2] |= (ata_id_wcache_enabled(id) << 2); /* write 
> cache enable */
> +   buf[12] |= (!ata_id_rahead_enabled(id) << 5);   /* disable 
> read ahead */
> +   }
> return sizeof(def_cache_mpage);
>  }
>
> @@ -2446,8 +2448,13 @@ static unsigned int ata_msense_control(struct 
> ata_device *dev, u8 *buf,
> bool changeable)
>  {
> modecpy(buf, def_control_mpage, sizeof(def_control_mpage), 
> changeable);
> -   if (changeable || (dev->flags & ATA_DFLAG_D_SENSE))
> -   buf[2] |= (1 << 2); /* Descriptor sense requested */
> +   if (changeable) {
> +   buf[2] |= (1 << 2); /* ata_mselect_control() */
> +   } else {
> +   bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
> +
> +   buf[2] |= (d_sense << 2);   /* descriptor format sense 
> data */
> +   }
> return sizeof(def_control_mpage);
>  }
>
> --
> 2.9.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] libata-scsi: better style in ata_msense_*()

[tom . ty89]

From: Tom Yan 

`changeable` is the "version" of mode page requested by the user.
It will be less confusing/misleading if we do not check it
"together" with the setting bits of the drive.

Not to mention that we currently have ata_mselect_*() implemented
in a way that each of them will serve exclusively a particular bit
on each page. The old style will hence make the condition look even
more unnecessarily arcane if the ata_msense_*() is reflecting more
than one bit.

Signed-off-by: Tom Yan 

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bf4cb21..b00521b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2424,10 +2424,12 @@ static void modecpy(u8 *dest, const u8 *src, int n, 
bool changeable)
 static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable)
 {
modecpy(buf, def_cache_mpage, sizeof(def_cache_mpage), changeable);
-   if (changeable || ata_id_wcache_enabled(id))
-   buf[2] |= (1 << 2); /* write cache enable */
-   if (!changeable && !ata_id_rahead_enabled(id))
-   buf[12] |= (1 << 5);/* disable read ahead */
+   if (changeable) {
+   buf[2] |= (1 << 2); /* ata_mselect_caching() */
+   } else {
+   buf[2] |= (ata_id_wcache_enabled(id) << 2); /* write cache 
enable */
+   buf[12] |= (!ata_id_rahead_enabled(id) << 5);   /* disable read 
ahead */
+   }
return sizeof(def_cache_mpage);
 }
 
@@ -2446,8 +2448,13 @@ static unsigned int ata_msense_control(struct ata_device 
*dev, u8 *buf,
bool changeable)
 {
modecpy(buf, def_control_mpage, sizeof(def_control_mpage), changeable);
-   if (changeable || (dev->flags & ATA_DFLAG_D_SENSE))
-   buf[2] |= (1 << 2); /* Descriptor sense requested */
+   if (changeable) {
+   buf[2] |= (1 << 2); /* ata_mselect_control() */
+   } else {
+   bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
+
+   buf[2] |= (d_sense << 2);   /* descriptor format sense data 
*/
+   }
return sizeof(def_control_mpage);
 }
 
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html