Re: [PATCH v5 1/6] ata: libata-scsi: add atapi_max_lun module parameter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unnecessary introduction of a global module parameter for a hardware quirk.
- [Medium] Off-by-one mismatch between the `atapi_max_lun` parameter description and its application as an exclusive upper bound.
--

commit 4a1228303dd79171832c9f9e8bbe33af4d0fc449
Author: Phil Pemberton <philpem@philpem.me.uk>

ata: libata-scsi: add atapi_max_lun module parameter

This commit introduces the atapi_max_lun module parameter to control the
upper bound of the per-host SCSI LUN scan for ATAPI devices. This enables
scanning past LUN 0 for multi-LUN devices.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e76d15411e2ae..4408b1fb48c77 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
[ ... ]
> @@ -122,6 +122,11 @@ int atapi_passthru16 = 1;
>  module_param(atapi_passthru16, int, 0444);
>  MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>  
> +int atapi_max_lun = 1;
> +module_param(atapi_max_lun, int, 0444);
> +MODULE_PARM_DESC(atapi_max_lun,
> +	"Maximum LUN to scan on ATAPI devices flagged BLIST_FORCELUN (1 [default] .. 7)");

Is this module parameter strictly necessary?

The commit message notes that LUN>0 probing is securely gated behind the
BLIST_FORCELUN SCSI device quirk. Because this safeguard exists, requiring a
global kernel command-line parameter in addition to the quirk seems redundant
and defeats the automatic detection philosophy of the quirk system.

Could the code dynamically increase shost->max_lun when the BLIST_FORCELUN
quirk is detected on LUN 0, eliminating the need for a global parameter?

Furthermore, does the documentation introduce an off-by-one mismatch?

The parameter is documented as "Maximum LUN to scan ... (1 [default] .. 7)",
which implies an inclusive maximum LUN ID. However, this value is assigned
directly to shost->max_lun:

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index f44612e269a4d..32c6a0e497cf9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -4627,7 +4627,7 @@ int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *s
>  		shost->transportt = &ata_scsi_transportt;
>  		shost->unique_id = ap->print_id;
>  		shost->max_id = 16;
> -		shost->max_lun = 1;
> +		shost->max_lun = clamp(atapi_max_lun, 1, ATAPI_MAX_LUN);

Because the SCSI midlayer treats shost->max_lun as an exclusive limit,
setting atapi_max_lun=7 based on the documentation will cause a regression
where the scanner stops at LUN 6, completely missing LUN 7.

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 5c085ef4eda7b..3e33ee30628d0 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
[ ... ]
> @@ -131,6 +131,7 @@ enum {
>  	ATA_SHORT_PAUSE		= 16,
>  
>  	ATAPI_MAX_DRAIN		= 16 << 10,
> +	ATAPI_MAX_LUN		= 8,	/* SCSI-2 cap (LUN values 0..7) */

Since the clamp function bounds the value to ATAPI_MAX_LUN (which is 8), the
value 8 is required to scan all 8 SCSI-2 LUNs, making the documentation
suggesting 7 as the maximum misleading.

Should the documentation be updated to clarify that it controls the total
number of LUNs (an exclusive bound), or should the parameter be treated as an
inclusive maximum?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512202728.299414-1-philpem@philpem.me.uk?part=1