Hi Alexey,
On 2026-09-18T06:35:41, Alexey Charkov <[email protected]> wrote:
> scsi: size a well known unit from the unit it stands in for
>
> A well known unit can read and write fine while refusing READ CAPACITY,
> because it presents another unit's medium rather than one of its own. The
> scan wrote such a unit off, since it has no geometry to build a block
> device from.
>
> Ask the controller which unit it currently presents and take the geometry
> from there, leaving the block device addressed by the well known unit so
> that the device keeps resolving where a request lands.
>
> Note that in cases where this matters (e.g. UFS boot W-LU) the physical
> underlying units should anyway have identical geometry to be usable in
> the UFS A/B boot scheme, so switching the active unit should not affect
> the geometry for real-world use.
>
> Signed-off-by: Alexey Charkov <[email protected]>
>
> drivers/scsi/scsi-uclass.c | 10 ++++++++++
> drivers/scsi/scsi.c | 31 +++++++++++++++++++++++++------
> include/scsi.h | 25 +++++++++++++++++++++++++
> 3 files changed, 60 insertions(+), 6 deletions(-)
> diff --git a/drivers/scsi/scsi-uclass.c b/drivers/scsi/scsi-uclass.c
> @@ -50,6 +50,16 @@ int scsi_get_blk_by_uuid(const char *uuid,
> +int scsi_wlun_alias(struct udevice *dev, int lun)
> +{
> + struct scsi_ops *ops = scsi_get_ops(dev);
> +
> + if (!ops->wlun_alias)
> + return -ENOSYS;
> +
> + return ops->wlun_alias(dev, lun);
> +}
Since you are adding a new uclass operation you need a test for it.
The sandbox SCSI driver currently rejects any non-zero LUN, but it
could declare a well known unit which refuses READ CAPACITY and
implement wlun_alias() to point at LUN 0. Then test/dm/scsi.c can
check that the block device appears with the borrowed geometry. That
would cover the scan and lookup in the next two patches as well.
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> @@ -511,7 +512,7 @@ static bool scsi_unit_attention(struct scsi_cmd *pccb)
> -static int scsi_detect_dev(struct udevice *dev, int target, int lun,
> +static int scsi_detect_dev(struct udevice *dev, int target, int lun, bool
> wlun,
> struct blk_desc *dev_desc)
Nothing passes true until the next patch, so the new path is dead code
here. Do you need this parameter at all? The driver already knows
which LUNs are well known (yours returns -ENOENT for anything other
than the boot W-LU), so you could call scsi_wlun_alias() whenever READ
CAPACITY fails and leave both function signatures alone. What do you
think?
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> @@ -584,8 +585,25 @@ static int scsi_detect_dev(struct udevice *dev, int
> target, int lun,
> + if (alias < 0) {
> + scsi_print_error(pccb);
> + return -EINVAL;
> + }
> +
> + pccb->lun = alias;
> + err = scsi_read_capacity(dev, pccb, &capacity, &blksz);
> + pccb->lun = lun;
> + if (err) {
> + scsi_print_error(pccb);
> + return -EINVAL;
> + }
The error path is duplicated and the comment about borrowing the
geometry sits above the failure check rather than the retry. How about
something like this:
err = scsi_read_capacity(dev, pccb, &capacity, &blksz);
if (err && wlun) {
alias = scsi_wlun_alias(dev, lun);
if (alias >= 0) {
pccb->lun = alias;
err = scsi_read_capacity(dev, pccb, &capacity, &blksz);
pccb->lun = lun;
}
}
if (err) {
scsi_print_error(pccb);
return -EINVAL;
}
Also, the value from the driver is used as a LUN without any check. I
suspect it is worth rejecting anything >= max_lun so that a driver bug
cannot send a command to an arbitrary unit.
> diff --git a/include/scsi.h b/include/scsi.h
> @@ -296,6 +296,21 @@ struct scsi_ops {
> + * can take the geometry from there. Which unit that is may change over
> + * the life of the device, so it is resolved on each scan.
The block device keeps the size it got at scan time, so if the active
unit changes to one with a different size, the size is stale until the
next scan. The commit message covers this assumption, but please can
you mention it here too, since this is where a driver author will
look.
Regards,
Simon
