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

Hi Jean-Jacques,

On 24 March 2017 at 06:24, Jean-Jacques Hiblot jjhiblot@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@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