Re: [PATCH v10 14/15] ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ata: libata-scsi: manual scan of ATAPI targets bypasses OF node assignment
--

commit 8bad316476ffbbf5d3a2fdb9796c051181202764
Author: Phil Pemberton <philpem@philpem.me.uk>

ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices
    
After adding LUN 0 of an ATAPI device marked BLIST_FORCELUN, scan the
target's remaining LUNs using scsi_scan_target(). The scan is bounded by
shost->max_lun, set through atapi_max_lun.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index faf408acf65ec..bba2db2f84284 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -5647,6 +5662,37 @@ int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel,
>  	} else
>  		spin_unlock_irqrestore(ap->lock, flags);
>  
> +	if (!rc) {
> +		struct ata_link *link;
> +		struct ata_device *dev;
> +
> +		/* EH does not scan missing LUNs on an already attached target. */
> +		mutex_lock(&ap->scsi_scan_mutex);
> +		ata_for_each_link(link, ap, EDGE) {
> +			ata_for_each_dev(dev, link, ENABLED) {

[ ... ]

> +				scsi_scan_target(&shost->shost_gendev,
> +						 scan_channel, scan_id, lun,
> +						 SCSI_SCAN_MANUAL);
> +				ata_scsi_publish_sdevs(dev);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this manual scan bypass OF node assignment and multi-LUN scanning for
newly attached ATAPI targets?

If a user initiates a manual scan via sysfs (for example, using
echo "- - -" > /sys/class/scsi_host/hostX/scan), ata_scsi_user_scan()
schedules and waits for EH. EH configures the device and queues
hotplug_task (a delayed work).

Because ata_scsi_user_scan() continues immediately, it usually beats the
deferred hotplug_task and populates dev->sdev[0] here via
ata_scsi_publish_sdevs().

When hotplug_task subsequently runs ata_scsi_scan_host(), it skips the
remaining initialization due to the guard check in ata_scsi_scan_host():

drivers/ata/libata-scsi.c:ata_scsi_scan_host() {
    ...
			if (dev->sdev[0])
				continue;
    ...
}

Will this entirely bypass ata_scsi_assign_ofnode() and the new BLIST_FORCELUN
multi-LUN scan logic for the target, potentially affecting device tree
bindings and leaving secondary LUNs inaccessible?

> +			}
> +		}
> +		mutex_unlock(&ap->scsi_scan_mutex);
> +	}
> +
>  	return rc;
>  }

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