[PATCH 0/8] ata: libata-scsi: MODE SELECT fixes and cleanups

[PATCH 0/8] ata: libata-scsi: MODE SELECT fixes and cleanups

[Niklas Cassel]

This series fixes several problems in the MODE SELECT translation of
libata, most of them related to the FIELD POINTER reported in the sense
data when a MODE SELECT parameter list is rejected, and makes the MODE
SELECT mode page handlers significantly easier to follow.


Most of these bugs were caused by the code being more complicated than
it needs to be.

Each mode page has a fixed format, and may only be sent in that format:

  Caching mode page:             SPF=0, page_0 format (no SUBPAGE CODE)
  Control mode page:             SPF=0, page_0 format (no SUBPAGE CODE)
  ATA Feature Control mode page: SPF=1, sub_page format (SUBPAGE CODE F2h)

SAT defines the SPF bit of the Caching and Control mode pages as not
changeable and set to zero, and requires it to be set to one for the ATA
Feature Control mode page. A MODE SELECT that sets the SPF bit to a value
that does not match the mode page must therefore be rejected with
INVALID FIELD IN PARAMETER LIST, as required by SPC.


This makes it possible to simplify the code significantly, by passing
the whole mode page, starting with its mode page header, to the
handlers, just like the ata_msense_*() functions generate the whole mode
page. Offsets in the mode page buffer are then the byte numbers of the
mode page, as used in the mode page definitions of SPC, SBC and SAT, the
handlers can compare it with the mode page generated by the matching
ata_msense_*() function without adjusting offsets, and the conversion
done by ata_scsi_mode_select_xlat() becomes correct. This also allows
reporting an invalid PAGE LENGTH at the PAGE LENGTH field, which was not
part of the buffer before.


The series was tested in QEMU, by sending MODE SELECT(6) and MODE
SELECT(10) parameter lists to an AHCI disk via SG_IO and checking the
returned sense data, and by switching the cache_type of the disk.

The FIELD POINTER reported before and after this series, for MODE
SELECT(10) parameter lists without block descriptors unless noted
otherwise:

  Parameter list                                     Before After Correct
  Caching page, byte 3 changed                            9    11      11
  Caching page, byte 3 changed, MODE SELECT(6)            5     7       7
  Control page, byte 3 changed                            9    11      11
  ATA Feature Control page, CDL_CTRL 01b                  8    12      12
  CDL enable while NCQ priority is enabled                7    12      12
  Caching page, PAGE LENGTH too short                    25     9       9
  ATA Feature Control page, PAGE LENGTH too short        19    10      10
  Caching page, SUBPAGE CODE 01h                          8     9       9
  Control page, SUBPAGE CODE FFh                          8     9       9
  BLOCK DESCRIPTOR LENGTH 16                             30     6       6
  BLOCK DESCRIPTOR LENGTH 16, MODE SELECT(6)             23     3       3
  BLOCK DESCRIPTOR LENGTH 4                              18     6       6
  Caching page followed by Control page               65535    28      28
  Control page followed by 1 byte                  accepted    20      20
  ATA Feature Control page followed by 1 byte         65535    24      24
  Caching page, SPF=1, SUBPAGE CODE 00h            accepted     8       8
  Caching page, SPF=1, SUBPAGE CODE FFh            accepted     9       9

"accepted" means that the command completed without error. Before this
series, the Control mode page followed by 1 byte was applied, ignoring the
extra byte, and the ATA Feature Control mode page followed by 1 byte, which
requests enabling CDL, updated the CDL enabled flag of the device even
though the command was rejected. As only one mode page per command is
supported, data after the mode page is rejected at its first byte.

These patches were developed with the help of an AI coding assistant.


Niklas Cassel (7):
  ata: libata-scsi: fix MODE SELECT handling of data after the mode page
  ata: libata-scsi: do not accept subpage code FFh in MODE SELECT
  ata: libata-scsi: reject MODE SELECT mode pages with a wrong SPF bit
  ata: libata-scsi: fix MODE SELECT field pointer for unsupported
    subpages
  ata: libata-scsi: fix MODE SELECT field pointer for block descriptor
    length
  ata: libata-scsi: pass the whole mode page to the MODE SELECT handlers
  ata: libata-scsi: document the MODE SENSE mode page functions

Shao Mingyin (1):
  ata: libata-scsi: set the field pointer when rejecting CDL

 drivers/ata/libata-scsi.c | 278 ++++++++++++++++++++++++++------------
 1 file changed, 195 insertions(+), 83 deletions(-)


base-commit: 6ba53556f93f79e5273d9a5fcd39c3ba3b0ea391
-- 
2.56.0

[PATCH 1/8] ata: libata-scsi: fix MODE SELECT handling of data after the mode page

[Niklas Cassel]

ata_scsi_mode_select_xlat() only supports one mode page per MODE SELECT
command, and rejects a parameter list with data after the mode page with
INVALID FIELD IN PARAMETER LIST. However:

- It does not set the field pointer, so the sense data reports 0xffff as
  the FIELD POINTER.

- It only checks for data after the mode page after the mode page
  handler has been called. ata_mselect_control_ata_feature() has then
  already updated the CDL enabled flag of the device, even though the
  command is rejected and the CDL setting of the device is not changed.

- For the control mode page, ata_mselect_control_spg0() does not need an
  ATA command, so the check is skipped, and the data after the mode page
  is silently ignored.

Check for data after the mode page before calling the mode page
handlers, and set the FIELD POINTER to the first byte after the mode
page, i.e. to the first byte of the next mode page.

The reason why only one mode page per command is supported is that a
MODE SELECT command is translated into at most one ATA command, so
update the comment accordingly.

Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a61fb0331279..7081caa8eb93 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4670,6 +4670,17 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 	if (pg_len > len)
 		goto invalid_param_len;
 
+	/*
+	 * A MODE SELECT command is translated into at most one ATA command, so
+	 * only one mode page per command is supported. Reject any data after
+	 * the mode page before calling the mode page handlers, which may
+	 * already update the device flags.
+	 */
+	if (len > pg_len) {
+		fp = p - buffer + pg_len;
+		goto invalid_param;
+	}
+
 	switch (pg) {
 	case CACHE_MPAGE:
 		if (ata_mselect_caching(qc, p, pg_len, &fp) < 0) {
@@ -4692,13 +4703,6 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param;
 	}
 
-	/*
-	 * Only one page has changeable data, so we only support setting one
-	 * page at a time.
-	 */
-	if (len > pg_len)
-		goto invalid_param;
-
 	return 0;
 
  invalid_fld:
-- 
2.56.0

Re: [PATCH 1/8] ata: libata-scsi: fix MODE SELECT handling of data after the mode page

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> ata_scsi_mode_select_xlat() only supports one mode page per MODE SELECT
> command, and rejects a parameter list with data after the mode page with
> INVALID FIELD IN PARAMETER LIST. However:
> 
> - It does not set the field pointer, so the sense data reports 0xffff as
>   the FIELD POINTER.
> 
> - It only checks for data after the mode page after the mode page
>   handler has been called. ata_mselect_control_ata_feature() has then
>   already updated the CDL enabled flag of the device, even though the
>   command is rejected and the CDL setting of the device is not changed.
> 
> - For the control mode page, ata_mselect_control_spg0() does not need an
>   ATA command, so the check is skipped, and the data after the mode page
>   is silently ignored.
> 
> Check for data after the mode page before calling the mode page
> handlers, and set the FIELD POINTER to the first byte after the mode
> page, i.e. to the first byte of the next mode page.
> 
> The reason why only one mode page per command is supported is that a
> MODE SELECT command is translated into at most one ATA command, so
> update the comment accordingly.
> 
> Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

[PATCH 2/8] ata: libata-scsi: do not accept subpage code FFh in MODE SELECT

[Niklas Cassel]

When the SUBPAGE CODE of the mode page in a MODE SELECT parameter list
is FFh, ata_scsi_mode_select_xlat() rejects it for the control mode
page, but accepts it for all other mode pages, i.e. for the caching
mode page, the only other mode page that it supports.

However, FFh is not the subpage code of an actual mode page: SPC-7
(7.5.2) only defines it as the MODE SENSE request code for all
subpages. SUBPAGE CODE FFh only means "all subpages" in requests, i.e.
in the MODE SENSE CDB for mode pages and in the LOG SELECT CDB for log
pages (see SPC-5, 6.8), while the SUBPAGE CODE of a mode page in a MODE
SELECT parameter list identifies a specific mode page. Also, SAT-2 and
later versions of SAT define the Caching mode page with the SPF bit set
to zero (see SAT-6, 10.4.9), i.e. without a SUBPAGE CODE field.

Remove the ALL_SUB_MPAGES case, so that SUBPAGE CODE FFh is rejected
like any other unsupported subpage.

Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7081caa8eb93..d7923bc664d6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4643,19 +4643,11 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 	}
 
 	/*
-	 * Supported subpages: all subpages and ATA feature sub-page f2h of
-	 * the control page.
+	 * The only supported subpage is the ATA feature sub-page f2h of the
+	 * control page.
 	 */
 	if (spg) {
 		switch (spg) {
-		case ALL_SUB_MPAGES:
-			/* All subpages is not supported for the control page */
-			if (pg == CONTROL_MPAGE) {
-				fp = (p[0] & 0x40) ? 1 : 0;
-				fp += hdr_len + bd_len;
-				goto invalid_param;
-			}
-			break;
 		case ATA_FEATURE_SUB_MPAGE:
 			if (qc->dev->flags & ATA_DFLAG_CDL &&
 			    pg == CONTROL_MPAGE)
-- 
2.56.0

Re: [PATCH 2/8] ata: libata-scsi: do not accept subpage code FFh in MODE SELECT

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> When the SUBPAGE CODE of the mode page in a MODE SELECT parameter list
> is FFh, ata_scsi_mode_select_xlat() rejects it for the control mode
> page, but accepts it for all other mode pages, i.e. for the caching
> mode page, the only other mode page that it supports.
> 
> However, FFh is not the subpage code of an actual mode page: SPC-7
> (7.5.2) only defines it as the MODE SENSE request code for all
> subpages. SUBPAGE CODE FFh only means "all subpages" in requests, i.e.
> in the MODE SENSE CDB for mode pages and in the LOG SELECT CDB for log
> pages (see SPC-5, 6.8), while the SUBPAGE CODE of a mode page in a MODE
> SELECT parameter list identifies a specific mode page. Also, SAT-2 and
> later versions of SAT define the Caching mode page with the SPF bit set
> to zero (see SAT-6, 10.4.9), i.e. without a SUBPAGE CODE field.
> 
> Remove the ALL_SUB_MPAGES case, so that SUBPAGE CODE FFh is rejected
> like any other unsupported subpage.
> 
> Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 3/8] ata: libata-scsi: reject MODE SELECT mode pages with a wrong SPF bit

[Niklas Cassel]

ata_scsi_mode_select_xlat() accepts the caching and control mode pages
both in page_0 format and in sub_page format with SUBPAGE CODE 00h.

However, SAT-2 and later versions of SAT define the SPF bit of the
Caching mode page and of the Control mode page as not changeable and
set to zero (see SAT-6, 10.4.9 and 10.4.6), and SPC-7 (6.12, MODE
SELECT(10) command) requires the device server to terminate a MODE
SELECT command with INVALID FIELD IN PARAMETER LIST if the application
client sets any field that is reported as not changeable to a value
other than its current value.

Reject a mode page with the SPF bit set to one and SUBPAGE CODE 00h,
with the FIELD POINTER set to the byte containing the SPF bit. The only
mode page with the SPF bit set to one that remains accepted is the ATA
Feature Control mode page (control mode page, sub-page f2h), for which
SAT-6 (12.3.4) requires the SPF bit to be set to one. The opposite case
does not need a check: a mode page in page_0 format always has subpage
code 00h, so it can never be the ATA Feature Control mode page.

This also allows the mode page handlers to rely on the format of their
mode page, which a later patch will make use of.

Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d7923bc664d6..7b0a02903fda 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4629,6 +4629,16 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 			goto invalid_param_len;
 
 		spg = p[1];
+		/*
+		 * Mode pages with subpage code 00h use the page_0 format: SAT
+		 * defines the SPF bit of the Caching and Control mode pages,
+		 * for subpage code 00h, as not changeable and set to zero.
+		 */
+		if (!spg) {
+			fp = hdr_len + bd_len;
+			goto invalid_param;
+		}
+
 		pg_len = get_unaligned_be16(&p[2]);
 		p += 4;
 		len -= 4;
-- 
2.56.0

Re: [PATCH 3/8] ata: libata-scsi: reject MODE SELECT mode pages with a wrong SPF bit

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> ata_scsi_mode_select_xlat() accepts the caching and control mode pages
> both in page_0 format and in sub_page format with SUBPAGE CODE 00h.
> 
> However, SAT-2 and later versions of SAT define the SPF bit of the
> Caching mode page and of the Control mode page as not changeable and
> set to zero (see SAT-6, 10.4.9 and 10.4.6), and SPC-7 (6.12, MODE
> SELECT(10) command) requires the device server to terminate a MODE
> SELECT command with INVALID FIELD IN PARAMETER LIST if the application
> client sets any field that is reported as not changeable to a value
> other than its current value.
> 
> Reject a mode page with the SPF bit set to one and SUBPAGE CODE 00h,
> with the FIELD POINTER set to the byte containing the SPF bit. The only
> mode page with the SPF bit set to one that remains accepted is the ATA
> Feature Control mode page (control mode page, sub-page f2h), for which
> SAT-6 (12.3.4) requires the SPF bit to be set to one. The opposite case
> does not need a check: a mode page in page_0 format always has subpage
> code 00h, so it can never be the ATA Feature Control mode page.
> 
> This also allows the mode page handlers to rely on the format of their
> mode page, which a later patch will make use of.
> 
> Fixes: 1b26d29ccd59 ("[libata] scsi: implement MODE SELECT command")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d7923bc664d6..7b0a02903fda 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4629,6 +4629,16 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>  			goto invalid_param_len;
>  
>  		spg = p[1];
> +		/*
> +		 * Mode pages with subpage code 00h use the page_0 format: SAT
> +		 * defines the SPF bit of the Caching and Control mode pages,
> +		 * for subpage code 00h, as not changeable and set to zero.
> +		 */
> +		if (!spg) {
> +			fp = hdr_len + bd_len;
> +			goto invalid_param;
> +		}

The comment block in-between the assignment and test is odd. Can you move the
assignment right before the if, after the comment block? That makes the code a
lot prettier :)

> +
>  		pg_len = get_unaligned_be16(&p[2]);
>  		p += 4;
>  		len -= 4;


-- 
Damien Le Moal
Western Digital Research

[PATCH 4/8] ata: libata-scsi: fix MODE SELECT field pointer for unsupported subpages

[Niklas Cassel]

When the mode page of a MODE SELECT parameter list has an unsupported
SUBPAGE CODE, ata_scsi_mode_select_xlat() sets the FIELD POINTER of the
INVALID FIELD IN PARAMETER LIST sense data to the SUBPAGE CODE byte of
the mode page header (byte 1 of the page) if the SPF bit is set, and to
the PAGE CODE byte (byte 0 of the page) otherwise.

However, the SPF bit is tested on p[0] after p has already been advanced
past the mode page header, so the test reads bit 6 of the first byte of
the mode page data instead. If the parameter list ends with the mode
page header, it even reads the byte that follows the parameter list in
buffer, which is not initialized when no data beyond the parameter list
was transferred.

The test is not needed anyway: spg can only be non-zero for a mode page
in sub_page format, i.e. with the SPF bit set, and the field in error is
the SUBPAGE CODE. Always set the FIELD POINTER to the SUBPAGE CODE byte,
which follows the mode parameter header, the block descriptors and the
PAGE CODE byte.

Fixes: 7780081c1f04 ("libata-scsi: Set information sense field for invalid parameter")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7b0a02903fda..782f31feee0a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4664,8 +4664,7 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 				break;
 			fallthrough;
 		default:
-			fp = (p[0] & 0x40) ? 1 : 0;
-			fp += hdr_len + bd_len;
+			fp = hdr_len + bd_len + 1;
 			goto invalid_param;
 		}
 	}
-- 
2.56.0

Re: [PATCH 4/8] ata: libata-scsi: fix MODE SELECT field pointer for unsupported subpages

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> When the mode page of a MODE SELECT parameter list has an unsupported
> SUBPAGE CODE, ata_scsi_mode_select_xlat() sets the FIELD POINTER of the
> INVALID FIELD IN PARAMETER LIST sense data to the SUBPAGE CODE byte of
> the mode page header (byte 1 of the page) if the SPF bit is set, and to
> the PAGE CODE byte (byte 0 of the page) otherwise.
> 
> However, the SPF bit is tested on p[0] after p has already been advanced
> past the mode page header, so the test reads bit 6 of the first byte of
> the mode page data instead. If the parameter list ends with the mode
> page header, it even reads the byte that follows the parameter list in
> buffer, which is not initialized when no data beyond the parameter list
> was transferred.
> 
> The test is not needed anyway: spg can only be non-zero for a mode page
> in sub_page format, i.e. with the SPF bit set, and the field in error is
> the SUBPAGE CODE. Always set the FIELD POINTER to the SUBPAGE CODE byte,
> which follows the mode parameter header, the block descriptors and the
> PAGE CODE byte.
> 
> Fixes: 7780081c1f04 ("libata-scsi: Set information sense field for invalid parameter")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 5/8] ata: libata-scsi: fix MODE SELECT field pointer for block descriptor length

[Niklas Cassel]

ata_scsi_mode_select_xlat() only accepts MODE SELECT parameter lists
without block descriptors or with a single 8 byte block descriptor. For
any other BLOCK DESCRIPTOR LENGTH, it terminates the command with
INVALID FIELD IN PARAMETER LIST, and the FIELD POINTER should point at
the BLOCK DESCRIPTOR LENGTH field, i.e. at byte 3 of the mode parameter
header(6) or at byte 6 of the mode parameter header(10), as SPC requires
the FIELD POINTER of a multiple-byte field to point at its first byte.

However, the mode parameter header length and the BLOCK DESCRIPTOR
LENGTH are added to the offset of the field, so the FIELD POINTER points
3 or 6 bytes after the end of the block descriptors instead. For
example, a MODE SELECT(10) with a BLOCK DESCRIPTOR LENGTH of 16 reports
byte 30 instead of byte 6.

Do not add the mode parameter header length and the BLOCK DESCRIPTOR
LENGTH to the offset of the BLOCK DESCRIPTOR LENGTH field.

Fixes: 7780081c1f04 ("libata-scsi: Set information sense field for invalid parameter")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 782f31feee0a..4b8454335aef 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4613,7 +4613,6 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param_len;
 	if (bd_len != 0 && bd_len != 8) {
 		fp = (six_byte) ? 3 : 6;
-		fp += bd_len + hdr_len;
 		goto invalid_param;
 	}
 
-- 
2.56.0

Re: [PATCH 5/8] ata: libata-scsi: fix MODE SELECT field pointer for block descriptor length

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> ata_scsi_mode_select_xlat() only accepts MODE SELECT parameter lists
> without block descriptors or with a single 8 byte block descriptor. For
> any other BLOCK DESCRIPTOR LENGTH, it terminates the command with
> INVALID FIELD IN PARAMETER LIST, and the FIELD POINTER should point at
> the BLOCK DESCRIPTOR LENGTH field, i.e. at byte 3 of the mode parameter
> header(6) or at byte 6 of the mode parameter header(10), as SPC requires
> the FIELD POINTER of a multiple-byte field to point at its first byte.
> 
> However, the mode parameter header length and the BLOCK DESCRIPTOR
> LENGTH are added to the offset of the field, so the FIELD POINTER points
> 3 or 6 bytes after the end of the block descriptors instead. For
> example, a MODE SELECT(10) with a BLOCK DESCRIPTOR LENGTH of 16 reports
> byte 30 instead of byte 6.
> 
> Do not add the mode parameter header length and the BLOCK DESCRIPTOR
> LENGTH to the offset of the BLOCK DESCRIPTOR LENGTH field.
> 
> Fixes: 7780081c1f04 ("libata-scsi: Set information sense field for invalid parameter")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 6/8] ata: libata-scsi: pass the whole mode page to the MODE SELECT handlers

[Niklas Cassel]

ata_scsi_mode_select_xlat() strips the mode page header before calling
the mode page handlers, which report the position of an invalid field
as an offset relative to the mode page data. It then converts this
offset into an offset in the parameter list by adding the length of the
mode parameter header and of the block descriptors, but not the length
of the mode page header. As a result, the FIELD POINTER of the INVALID
FIELD IN PARAMETER LIST sense data points 2 bytes (page_0 format) or 4
bytes (sub_page format) before the byte that is actually invalid. For
example, changing byte 3 of the caching mode page in a MODE SELECT(10)
parameter list without block descriptors reports byte 9 instead of
byte 11.

Pass the whole mode page, starting with its mode page header, to the
handlers instead, like the ata_msense_*() functions, which generate the
whole mode page. Offsets in buf are then the byte numbers of the mode
page, as used in the mode page definitions of SPC, SBC and SAT, the
handlers can compare buf with the mode page generated by the matching
ata_msense_*() function without adjusting offsets, and the conversion
done by ata_scsi_mode_select_xlat() becomes correct. This also allows
reporting an invalid PAGE LENGTH at the PAGE LENGTH field, which was not
part of buf before. As ata_scsi_mode_select_xlat() rejects SUBPAGE CODE
FFh and mode pages with a wrong SPF bit, each handler only gets its mode
page in a single format, so the mode page header length is known.

Document what buf, len and fp are, and the return values, in kernel-doc
comments for all the handlers.

Fixes: 7780081c1f04 ("libata-scsi: Set information sense field for invalid parameter")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 133 +++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 51 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 4b8454335aef..3257bbbba5b6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4366,14 +4366,22 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 /**
  *	ata_mselect_caching - Simulate MODE SELECT for caching info page
  *	@qc: Storage for translated ATA taskfile
- *	@buf: input buffer
- *	@len: number of valid bytes in the input buffer
- *	@fp: out parameter for the failed field on error
+ *	@buf: Mode page, starting with the mode page header
+ *	@len: Length of the mode page, including the mode page header
+ *	@fp: Out parameter, offset in @buf of the first invalid byte on error
  *
  *	Prepare a taskfile to modify caching information for the device.
  *
+ *	The caller has stripped the mode parameter header and the block
+ *	descriptors, so @buf points at the mode page header, like the buffer
+ *	filled by the ata_msense_*() functions, and offsets in @buf, including
+ *	the one returned in @fp, are the byte numbers of the mode page.
+ *
  *	LOCKING:
  *	None.
+ *
+ *	RETURNS:
+ *	0 if a taskfile was prepared, -EINVAL if the mode page is invalid.
  */
 static int ata_mselect_caching(struct ata_queued_cmd *qc,
 			       const u8 *buf, int len, u16 *fp)
@@ -4384,26 +4392,19 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	u8 wce;
 	int i;
 
-	/*
-	 * The first two bytes of def_cache_mpage are a header, so offsets
-	 * in mpage are off by 2 compared to buf.  Same for len.
-	 */
-
-	if (len != CACHE_MPAGE_LEN - 2) {
-		*fp = min(len, CACHE_MPAGE_LEN - 2);
+	if (len != CACHE_MPAGE_LEN) {
+		*fp = 1;
 		return -EINVAL;
 	}
 
-	wce = buf[0] & (1 << 2);
+	wce = buf[2] & (1 << 2);
 
 	/*
 	 * Check that read-only bits are not modified.
 	 */
 	ata_msense_caching(dev->id, mpage, false);
-	for (i = 0; i < CACHE_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
-		if (mpage[i + 2] != buf[i]) {
+	for (i = 3; i < CACHE_MPAGE_LEN; i++) {
+		if (mpage[i] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
 		}
@@ -4417,8 +4418,26 @@ static int ata_mselect_caching(struct ata_queued_cmd *qc,
 	return 0;
 }
 
-/*
- * Simulate MODE SELECT control mode page, sub-page 0.
+/**
+ *	ata_mselect_control_spg0 - Simulate MODE SELECT for control sub-page 0
+ *	@qc: Storage for translated ATA taskfile
+ *	@buf: Mode page, starting with the mode page header
+ *	@len: Length of the mode page, including the mode page header
+ *	@fp: Out parameter, offset in @buf of the first invalid byte on error
+ *
+ *	Simulate MODE SELECT control mode page, sub-page 0.
+ *
+ *	The caller has stripped the mode parameter header and the block
+ *	descriptors, so @buf points at the mode page header, like the buffer
+ *	filled by the ata_msense_*() functions, and offsets in @buf, including
+ *	the one returned in @fp, are the byte numbers of the mode page.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	0, as no ATA command needs to be sent, or -EINVAL if the mode page is
+ *	invalid.
  */
 static int ata_mselect_control_spg0(struct ata_queued_cmd *qc,
 				    const u8 *buf, int len, u16 *fp)
@@ -4428,26 +4447,19 @@ static int ata_mselect_control_spg0(struct ata_queued_cmd *qc,
 	u8 d_sense;
 	int i;
 
-	/*
-	 * The first two bytes of def_control_mpage are a header, so offsets
-	 * in mpage are off by 2 compared to buf.  Same for len.
-	 */
-
-	if (len != CONTROL_MPAGE_LEN - 2) {
-		*fp = min(len, CONTROL_MPAGE_LEN - 2);
+	if (len != CONTROL_MPAGE_LEN) {
+		*fp = 1;
 		return -EINVAL;
 	}
 
-	d_sense = buf[0] & (1 << 2);
+	d_sense = buf[2] & (1 << 2);
 
 	/*
 	 * Check that read-only bits are not modified.
 	 */
 	ata_msense_control_spg0(dev, mpage, false);
-	for (i = 0; i < CONTROL_MPAGE_LEN - 2; i++) {
-		if (i == 0)
-			continue;
-		if (mpage[2 + i] != buf[i]) {
+	for (i = 3; i < CONTROL_MPAGE_LEN; i++) {
+		if (mpage[i] != buf[i]) {
 			*fp = i;
 			return -EINVAL;
 		}
@@ -4459,9 +4471,27 @@ static int ata_mselect_control_spg0(struct ata_queued_cmd *qc,
 	return 0;
 }
 
-/*
- * Translate MODE SELECT control mode page, sub-page f2h (ATA feature mode
- * page) into a SET FEATURES command.
+/**
+ *	ata_mselect_control_ata_feature - Translate ATA Feature Control page
+ *	@qc: Storage for translated ATA taskfile
+ *	@buf: Mode page, starting with the mode page header
+ *	@len: Length of the mode page, including the mode page header
+ *	@fp: Out parameter, offset in @buf of the first invalid byte on error
+ *
+ *	Translate MODE SELECT control mode page, sub-page f2h (ATA feature mode
+ *	page) into a SET FEATURES command.
+ *
+ *	The caller has stripped the mode parameter header and the block
+ *	descriptors, so @buf points at the mode page header, like the buffer
+ *	filled by the ata_msense_*() functions, and offsets in @buf, including
+ *	the one returned in @fp, are the byte numbers of the mode page.
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	1 if a taskfile was prepared, -EINVAL if the mode page is invalid or if
+ *	CDL cannot be enabled.
  */
 static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 					   const u8 *buf, int len, u16 *fp)
@@ -4470,17 +4500,13 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 	struct ata_taskfile *tf = &qc->tf;
 	u8 cdl_action;
 
-	/*
-	 * The first four bytes of ATA Feature Control mode page are a header,
-	 * so offsets in mpage are off by 4 compared to buf.  Same for len.
-	 */
-	if (len != ATA_FEATURE_SUB_MPAGE_LEN - 4) {
-		*fp = min(len, ATA_FEATURE_SUB_MPAGE_LEN - 4);
+	if (len != ATA_FEATURE_SUB_MPAGE_LEN) {
+		*fp = 2;
 		return -EINVAL;
 	}
 
 	/* Check cdl_ctrl */
-	switch (buf[0] & 0x03) {
+	switch (buf[4] & 0x03) {
 	case 0:
 		/* Disable CDL */
 		ata_dev_dbg(dev, "Disabling CDL\n");
@@ -4502,7 +4528,7 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 		dev->flags |= ATA_DFLAG_CDL_ENABLED;
 		break;
 	default:
-		*fp = 0;
+		*fp = 4;
 		return -EINVAL;
 	}
 
@@ -4518,15 +4544,24 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 /**
  *	ata_mselect_control - Simulate MODE SELECT for control page
  *	@qc: Storage for translated ATA taskfile
- *	@spg: target sub-page of the control page
- *	@buf: input buffer
- *	@len: number of valid bytes in the input buffer
- *	@fp: out parameter for the failed field on error
+ *	@spg: Sub-page of the control page, 0 or ATA_FEATURE_SUB_MPAGE
+ *	@buf: Mode page, starting with the mode page header
+ *	@len: Length of the mode page, including the mode page header
+ *	@fp: Out parameter, offset in @buf of the first invalid byte on error
  *
- *	Prepare a taskfile to modify caching information for the device.
+ *	Call the handler of sub-page @spg of the control mode page.
+ *
+ *	The caller has stripped the mode parameter header and the block
+ *	descriptors, so @buf points at the mode page header, like the buffer
+ *	filled by the ata_msense_*() functions, and offsets in @buf, including
+ *	the one returned in @fp, are the byte numbers of the mode page.
  *
  *	LOCKING:
  *	None.
+ *
+ *	RETURNS:
+ *	0 if no ATA command needs to be sent, 1 if a taskfile was prepared,
+ *	-EINVAL if the mode page is invalid.
  */
 static int ata_mselect_control(struct ata_queued_cmd *qc, u8 spg,
 			       const u8 *buf, int len, u16 *fp)
@@ -4638,17 +4673,13 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 			goto invalid_param;
 		}
 
-		pg_len = get_unaligned_be16(&p[2]);
-		p += 4;
-		len -= 4;
+		pg_len = get_unaligned_be16(&p[2]) + 4;
 	} else {
 		if (len < 2)
 			goto invalid_param_len;
 
 		spg = 0;
-		pg_len = p[1];
-		p += 2;
-		len -= 2;
+		pg_len = p[1] + 2;
 	}
 
 	/*
-- 
2.56.0

Re: [PATCH 6/8] ata: libata-scsi: pass the whole mode page to the MODE SELECT handlers

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> ata_scsi_mode_select_xlat() strips the mode page header before calling
> the mode page handlers, which report the position of an invalid field
> as an offset relative to the mode page data. It then converts this
> offset into an offset in the parameter list by adding the length of the
> mode parameter header and of the block descriptors, but not the length
> of the mode page header. As a result, the FIELD POINTER of the INVALID
> FIELD IN PARAMETER LIST sense data points 2 bytes (page_0 format) or 4
> bytes (sub_page format) before the byte that is actually invalid. For
> example, changing byte 3 of the caching mode page in a MODE SELECT(10)
> parameter list without block descriptors reports byte 9 instead of
> byte 11.
> 
> Pass the whole mode page, starting with its mode page header, to the
> handlers instead, like the ata_msense_*() functions, which generate the
> whole mode page. Offsets in buf are then the byte numbers of the mode
> page, as used in the mode page definitions of SPC, SBC and SAT, the
> handlers can compare buf with the mode page generated by the matching
> ata_msense_*() function without adjusting offsets, and the conversion
> done by ata_scsi_mode_select_xlat() becomes correct. This also allows
> reporting an invalid PAGE LENGTH at the PAGE LENGTH field, which was not
> part of buf before. As ata_scsi_mode_select_xlat() rejects SUBPAGE CODE
> FFh and mode pages with a wrong SPF bit, each handler only gets its mode
> page in a single format, so the mode page header length is known.
> 
> Document what buf, len and fp are, and the return values, in kernel-doc
> comments for all the handlers.
> 
> Fixes: 7780081c1f04 ("libata-scsi: Set information sense field for invalid parameter")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 7/8] ata: libata-scsi: set the field pointer when rejecting CDL

[Niklas Cassel]

From: Shao Mingyin <shao.mingyin@zte.com.cn>

ata_mselect_control_ata_feature() rejects enabling CDL while NCQ
priority is enabled, but unlike its other error branches it returns
without setting *fp.

ata_scsi_mode_select_xlat() initializes fp to 0xffff and, when the
handler fails, adds the offset of the mode page in the parameter list
to it, i.e. the length of the mode parameter header and of the block
descriptors. As fp is a u16, this wraps around, so the resulting sense
data reports the byte before the mode page, i.e. the last byte of the
mode parameter header or of the block descriptors, as the FIELD
POINTER, instead of the byte containing the CDL_CTRL field that
requests enabling CDL.

Set *fp to 4, i.e. to the byte containing the CDL_CTRL field, like the
branch rejecting an invalid CDL_CTRL value does.

Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
[cassel: set *fp to 4, the offset of CDL_CTRL in the mode page]
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3257bbbba5b6..e91920659c22 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4521,6 +4521,7 @@ static int ata_mselect_control_ata_feature(struct ata_queued_cmd *qc,
 		if (dev->flags & ATA_DFLAG_NCQ_PRIO_ENABLED) {
 			ata_dev_err(dev,
 				"NCQ priority must be disabled to enable CDL\n");
+			*fp = 4;
 			return -EINVAL;
 		}
 		ata_dev_dbg(dev, "Enabling CDL\n");
-- 
2.56.0

Re: [PATCH 7/8] ata: libata-scsi: set the field pointer when rejecting CDL

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> From: Shao Mingyin <shao.mingyin@zte.com.cn>
> 
> ata_mselect_control_ata_feature() rejects enabling CDL while NCQ
> priority is enabled, but unlike its other error branches it returns
> without setting *fp.
> 
> ata_scsi_mode_select_xlat() initializes fp to 0xffff and, when the
> handler fails, adds the offset of the mode page in the parameter list
> to it, i.e. the length of the mode parameter header and of the block
> descriptors. As fp is a u16, this wraps around, so the resulting sense
> data reports the byte before the mode page, i.e. the last byte of the
> mode parameter header or of the block descriptors, as the FIELD
> POINTER, instead of the byte containing the CDL_CTRL field that
> requests enabling CDL.
> 
> Set *fp to 4, i.e. to the byte containing the CDL_CTRL field, like the
> branch rejecting an invalid CDL_CTRL value does.
> 
> Fixes: df60f9c64576 ("scsi: ata: libata: Add ATA feature control sub-page translation")
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> [cassel: set *fp to 4, the offset of CDL_CTRL in the mode page]
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 8/8] ata: libata-scsi: document the MODE SENSE mode page functions

[Niklas Cassel]

The ata_msense_*() functions generate the mode pages returned by MODE
SENSE, and are also used by the MODE SELECT mode page handlers to check
the mode page data. They do not document where their buf argument points
and what they return, and some of them do not have a kernel-doc comment
at all.

Add kernel-doc comments for all of them, stating that the mode page is
written to buf starting with its mode page header, which matches how
the MODE SELECT mode page handlers get the mode page, and that they
return the length of the mode page, including the mode page header.

No functional change.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 100 +++++++++++++++++++++++++++++++++-----
 1 file changed, 88 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e91920659c22..efdaf20b0ca4 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2645,15 +2645,23 @@ static void modecpy(u8 *dest, const u8 *src, int n, bool changeable)
 /**
  *	ata_msense_caching - Simulate MODE SENSE caching info page
  *	@id: device IDENTIFY data
- *	@buf: output buffer
+ *	@buf: Output buffer for the mode page, including the mode page header
  *	@changeable: whether changeable parameters are requested
  *
  *	Generate a caching info page, which conditionally indicates
  *	write caching to the SCSI layer, depending on device
  *	capabilities.
  *
+ *	The mode page is written to @buf starting with its mode page header,
+ *	so offsets in @buf are the byte numbers of the mode page. For MODE
+ *	SENSE, the mode parameter header and the block descriptors are written
+ *	by ata_scsiop_mode_sense().
+ *
  *	LOCKING:
  *	None.
+ *
+ *	RETURNS:
+ *	Length of the mode page, including the mode page header.
  */
 static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable)
 {
@@ -2667,8 +2675,25 @@ static unsigned int ata_msense_caching(u16 *id, u8 *buf, bool changeable)
 	return sizeof(def_cache_mpage);
 }
 
-/*
- * Simulate MODE SENSE control mode page, sub-page 0.
+/**
+ *	ata_msense_control_spg0 - Simulate MODE SENSE for control sub-page 0
+ *	@dev: ATA device of interest
+ *	@buf: Output buffer for the mode page, including the mode page header
+ *	@changeable: whether changeable parameters are requested
+ *
+ *	Simulate MODE SENSE control mode page, sub-page 0, which reports the
+ *	sense data format (D_SENSE) used for the device.
+ *
+ *	The mode page is written to @buf starting with its mode page header,
+ *	so offsets in @buf are the byte numbers of the mode page. For MODE
+ *	SENSE, the mode parameter header and the block descriptors are written
+ *	by ata_scsiop_mode_sense().
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Length of the mode page, including the mode page header.
  */
 static unsigned int ata_msense_control_spg0(struct ata_device *dev, u8 *buf,
 					    bool changeable)
@@ -2700,9 +2725,27 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)
 	return min_t(u32, limit / 10000, 65535);
 }
 
-/*
- * Simulate MODE SENSE control mode page, sub-pages 07h and 08h
- * (command duration limits T2A and T2B mode pages).
+/**
+ *	ata_msense_control_spgt2 - Simulate MODE SENSE for CDL T2A/T2B pages
+ *	@dev: ATA device of interest
+ *	@buf: Output buffer for the mode page, including the mode page header
+ *	@spg: Sub-page code, CDL_T2A_SUB_MPAGE or CDL_T2B_SUB_MPAGE
+ *
+ *	Simulate MODE SENSE control mode page, sub-pages 07h and 08h
+ *	(command duration limits T2A and T2B mode pages), from the command
+ *	duration limit descriptors read from the device.
+ *
+ *	The mode page is written to @buf starting with its mode page header,
+ *	so offsets in @buf are the byte numbers of the mode page. For MODE
+ *	SENSE, the mode parameter header and the block descriptors are written
+ *	by ata_scsiop_mode_sense().
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Length of the mode page, including the mode page header, or 0 if the
+ *	device does not support command duration limits.
  */
 static unsigned int ata_msense_control_spgt2(struct ata_device *dev, u8 *buf,
 					     u8 spg)
@@ -2759,9 +2802,25 @@ static unsigned int ata_msense_control_spgt2(struct ata_device *dev, u8 *buf,
 	return CDL_T2_SUB_MPAGE_LEN;
 }
 
-/*
- * Simulate MODE SENSE control mode page, sub-page f2h
- * (ATA feature control mode page).
+/**
+ *	ata_msense_control_ata_feature - Simulate ATA Feature Control page
+ *	@dev: ATA device of interest
+ *	@buf: Output buffer for the mode page, including the mode page header
+ *
+ *	Simulate MODE SENSE control mode page, sub-page f2h (ATA feature
+ *	control mode page), which reports whether command duration limits are
+ *	enabled.
+ *
+ *	The mode page is written to @buf starting with its mode page header,
+ *	so offsets in @buf are the byte numbers of the mode page. For MODE
+ *	SENSE, the mode parameter header and the block descriptors are written
+ *	by ata_scsiop_mode_sense().
+ *
+ *	LOCKING:
+ *	None.
+ *
+ *	RETURNS:
+ *	Length of the mode page, including the mode page header.
  */
 static unsigned int ata_msense_control_ata_feature(struct ata_device *dev,
 						   u8 *buf)
@@ -2787,14 +2846,23 @@ static unsigned int ata_msense_control_ata_feature(struct ata_device *dev,
 /**
  *	ata_msense_control - Simulate MODE SENSE control mode page
  *	@dev: ATA device of interest
- *	@buf: output buffer
+ *	@buf: Output buffer for the mode pages, including the page headers
  *	@spg: sub-page code
  *	@changeable: whether changeable parameters are requested
  *
- *	Generate a generic MODE SENSE control mode page.
+ *	Generate sub-page @spg of the control mode page, or all supported
+ *	sub-pages of the control mode page if @spg is ALL_SUB_MPAGES.
+ *
+ *	The mode pages are written to @buf one after the other, each starting
+ *	with its mode page header. For MODE SENSE, the mode parameter header
+ *	and the block descriptors are written by ata_scsiop_mode_sense().
  *
  *	LOCKING:
  *	None.
+ *
+ *	RETURNS:
+ *	Total length of the mode pages written to @buf, including their mode
+ *	page headers.
  */
 static unsigned int ata_msense_control(struct ata_device *dev, u8 *buf,
 				       u8 spg, bool changeable)
@@ -2822,13 +2890,21 @@ static unsigned int ata_msense_control(struct ata_device *dev, u8 *buf,
 
 /**
  *	ata_msense_rw_recovery - Simulate MODE SENSE r/w error recovery page
- *	@buf: output buffer
+ *	@buf: Output buffer for the mode page, including the mode page header
  *	@changeable: whether changeable parameters are requested
  *
  *	Generate a generic MODE SENSE r/w error recovery page.
  *
+ *	The mode page is written to @buf starting with its mode page header,
+ *	so offsets in @buf are the byte numbers of the mode page. For MODE
+ *	SENSE, the mode parameter header and the block descriptors are written
+ *	by ata_scsiop_mode_sense().
+ *
  *	LOCKING:
  *	None.
+ *
+ *	RETURNS:
+ *	Length of the mode page, including the mode page header.
  */
 static unsigned int ata_msense_rw_recovery(u8 *buf, bool changeable)
 {
-- 
2.56.0

Re: [PATCH 8/8] ata: libata-scsi: document the MODE SENSE mode page functions

[Damien Le Moal]

On 11/4/26 05:54, Niklas Cassel wrote:
> The ata_msense_*() functions generate the mode pages returned by MODE
> SENSE, and are also used by the MODE SELECT mode page handlers to check
> the mode page data. They do not document where their buf argument points
> and what they return, and some of them do not have a kernel-doc comment
> at all.
> 
> Add kernel-doc comments for all of them, stating that the mode page is
> written to buf starting with its mode page header, which matches how
> the MODE SELECT mode page handlers get the mode page, and that they
> return the length of the mode page, including the mode page header.
> 
> No functional change.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research