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

[Jean-Jacques Hiblot <jjhiblot@ti.com>]

On 01/04/2017 06:22, Simon Glass wrote:
> 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.
I simply tried as much as possible to avoid code duplication. But agreed 
it's not a very elegant solution. I'll rework this. Thanks again for the 
comments

>>   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?
No. It's just a rebase issue. I started the work on v2017.01 and didn't 
see the introduction of the unbind.
>>          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
>