[U-Boot] [PATCH 6/7] dm: scsi: fix scan

[Simon Glass]

Hi Jean-Jacques,

On 24 March 2017 at 06:24, Jean-Jacques Hiblot <jjhiblot at ti.com> wrote:
> With DM_SCSI enabled, scsi scan suffers 2 problems:
> * blk_create_devicef is called with blkz = 0, leading to a divide-by-0
>   exception
> * new blk devices are created at each scan.
>
> To fix this we start by removing all known SCSI devices at the beginning
> of the scan. Then a detection process is started for each possible
> device only to get the blksz and lba of the device (no call to part_init() yet).
> If a device is found, we can call blk_create_devicef() with the right
> parameters and continue the process.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> ---
>  common/scsi.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/common/scsi.c b/common/scsi.c
> index fb5b407..3385cc2 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -480,7 +480,7 @@ static void scsi_init_dev_desc(struct blk_desc *dev_desc, int devnum)
>   *
>   * Return: 0 on success, error value otherwise
>   */
> -static int scsi_detect_dev(int target, struct blk_desc *dev_desc)
> +static int scsi_detect_dev(int target, struct blk_desc *dev_desc, int init_part)
>  {
>         unsigned char perq, modi;
>         lbaint_t capacity;
> @@ -539,7 +539,8 @@ static int scsi_detect_dev(int target, struct blk_desc *dev_desc)
>         dev_desc->blksz = blksz;
>         dev_desc->log2blksz = LOG2(dev_desc->blksz);
>         dev_desc->type = perq;
> -       part_init(&dev_desc[0]);
> +       if (init_part)
> +               part_init(&dev_desc[0]);

Do you need this in here? The caller could just do it and avoid the
extra parameter.

>  removable:
>         return 0;
>  }
> @@ -565,6 +566,9 @@ int scsi_scan(int mode)
>         if (ret)
>                 return ret;
>
> +       /* remove all SCSI interfaces since we're going to (re)create them */
> +       blk_unbind_all(IF_TYPE_SCSI);
> +

This seems to already be done a few lines up...is it needed?

>         uclass_foreach_dev(dev, uc) {
>                 struct scsi_platdata *plat; /* scsi controller platdata */
>
> @@ -580,9 +584,20 @@ int scsi_scan(int mode)
>                         for (lun = 0; lun < plat->max_lun; lun++) {
>                                 struct udevice *bdev; /* block device */
>                                 /* block device description */
> +                               struct blk_desc _bd;
>                                 struct blk_desc *bdesc;
>                                 char str[10];
>
> +                               scsi_init_dev_desc_priv(&_bd);
> +                               _bd.lun = lun;
> +                               ret = scsi_detect_dev(i, &_bd, 0);
> +                               if (ret)
> +                                       /*
> +                                        * no device detected?
> +                                        * check the next lun.
> +                                        */
> +                                       continue;
> +
>                                 /*
>                                  * Create only one block device and do detection
>                                  * to make sure that there won't be a lot of
> @@ -590,17 +605,25 @@ int scsi_scan(int mode)
>                                  */
>                                 snprintf(str, sizeof(str), "id%dlun%d", i, lun);
>                                 ret = blk_create_devicef(dev, "scsi_blk",
> -                                                         str, IF_TYPE_SCSI,
> -                                                         -1, 0, 0, &bdev);
> +                                               str, IF_TYPE_SCSI,
> +                                               -1,
> +                                               _bd.blksz,
> +                                               _bd.blksz * _bd.lba,
> +                                               &bdev);

But why put these in here when...

>                                 if (ret) {
>                                         debug("Can't create device\n");
>                                         return ret;
>                                 }
>                                 bdesc = dev_get_uclass_platdata(bdev);
>
> +                               /*
> +                                * perform the detection once more but this
> +                                * time, let's initialize do the initialization
> +                                * of the partitions
> +                                */
>                                 scsi_init_dev_desc_priv(bdesc);

...they are overwritten here?

>                                 bdesc->lun = lun;
> -                               ret = scsi_detect_dev(i, bdesc);
> +                               ret = scsi_detect_dev(i, bdesc, 1);
>                                 if (ret) {
>                                         device_unbind(bdev);
>                                         continue;
> @@ -631,7 +654,7 @@ int scsi_scan(int mode)
>         for (i = 0; i < CONFIG_SYS_SCSI_MAX_SCSI_ID; i++) {
>                 for (lun = 0; lun < CONFIG_SYS_SCSI_MAX_LUN; lun++) {
>                         scsi_dev_desc[scsi_max_devs].lun = lun;
> -                       ret = scsi_detect_dev(i, &scsi_dev_desc[scsi_max_devs]);
> +                       ret = scsi_detect_dev(i, &scsi_dev_desc[scsi_max_devs], 1);
>                         if (ret)
>                                 continue;

Call part_init() here?

>
> --
> 1.9.1
>

Regards,
Simon