Re: [PATCH v5 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array

[Hannes Reinecke]

On 5/12/26 22:27, Phil Pemberton wrote:
> Multi-LUN ATAPI devices (PD/CD combos, CD changers) share a single
> ata_device but expose multiple scsi_devices.  The previous single
> dev->sdev pointer could only track one LUN, making all other LUNs
> invisible to code that operates on sdevs: port detach, suspend/resume,
> ACPI uevent, ZPODD, media change notification, and EH teardown.
>
> Replace the scalar struct scsi_device *sdev with a fixed-size array
> dev->sdev[ATAPI_MAX_LUN] indexed by LUN number, where ATAPI_MAX_LUN
> is 8 (the SCSI-2 ceiling, LUN values 0..7).  Add a companion field
> dev->nr_luns recording the number of valid entries -- defaults to 1
> during ata_dev_init() and is bumped during multi-LUN probe -- so the
> common single-LUN case iterates one slot, not eight.
>
> Add an inline helper ata_dev_scsi_device(dev, lun) that returns
> dev->sdev[lun] guarded by a WARN_ON_ONCE(lun >= dev->nr_luns) bounds
> check.  Use it for the hardcoded LUN-0 references in libata-acpi
> (uevent kobj), libata-zpodd (disk events, wake notify), and the
> door-lock and OF-node paths in libata-scsi.
>
> Key changes per call site:
>    - ata_scsi_dev_config:  assign sdev to dev->sdev[sdev->lun]
>    - ata_scsi_sdev_destroy: clear dev->sdev[sdev->lun]; only trigger
>      ATA-level detach when LUN 0 is destroyed, since removing a higher
>      LUN should not tear down the underlying ATA device
>    - ata_port_detach:  iterate dev->nr_luns slots (high->low)
>    - ata_scsi_offline_dev:  iterate dev->nr_luns slots
>    - ata_scsi_remove_dev:  snapshot and remove all LUN slots, then
>      scsi_remove_device each one outside the lock
>    - ata_scsi_media_change_notify:  send event to all populated LUNs
>    - ata_scsi_dev_rescan:  resume and rescan each populated LUN
>    - ACPI, ZPODD, ofnode, door-lock:  use ata_dev_scsi_device(dev, 0)
>
> For single-LUN devices (the vast majority) only dev->sdev[0] is ever
> populated and dev->nr_luns stays at 1, so existing call paths see no
> change in behaviour.
>
> Signed-off-by: Phil Pemberton <philpem@xxxxxxxxxxxxx>
> ---
>   drivers/ata/libata-acpi.c  |   6 +-
>   drivers/ata/libata-core.c  |  11 ++-
>   drivers/ata/libata-scsi.c  | 151 +++++++++++++++++++------------------
>   drivers/ata/libata-zpodd.c |   6 +-
>   include/linux/libata.h     |  11 ++-
>   5 files changed, 103 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index 4433f626246b..8af35d0b1053 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -153,8 +153,10 @@ static void ata_acpi_uevent(struct ata_port *ap, struct ata_device *dev,
>   	char *envp[] = { event_string, NULL };
>   
>   	if (dev) {
> -		if (dev->sdev)
> -			kobj = &dev->sdev->sdev_gendev.kobj;
> +		struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
> +
> +		if (sdev)
> +			kobj = &sdev->sdev_gendev.kobj;
>   	} else
>   		kobj = &ap->dev->kobj;
>   
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 4408b1fb48c7..1cb159d9dbc7 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5564,6 +5564,7 @@ void ata_dev_init(struct ata_device *dev)
>   	dev->pio_mask = UINT_MAX;
>   	dev->mwdma_mask = UINT_MAX;
>   	dev->udma_mask = UINT_MAX;
> +	dev->nr_luns = 1;
>   }
>   
>   /**
> @@ -6275,11 +6276,15 @@ static void ata_port_detach(struct ata_port *ap)
>   	/* Remove scsi devices */
>   	ata_for_each_link(link, ap, HOST_FIRST) {
>   		ata_for_each_dev(dev, link, ALL) {
> -			if (dev->sdev) {
> +			int lun;
> +
> +			for (lun = dev->nr_luns - 1; lun >= 0; lun--) {
> +				if (!dev->sdev[lun])> +					continue;

That looks so weird.
The 'sdev' array has the size ATAPI_MAX_LUN, so ->nr_luns should
never be larger than that.
_And_ you are checking for the pointer directly.
So why do you have 'dev->nr_luns'?
Wouldn't it be easier to use ATAPI_MAX_LUN as the upper limit,
and do away with 'nr_luns' completely?

Cheers,

Hannes
--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@xxxxxxx                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich