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

[Jean-Jacques Hiblot]

On 01/04/2017 06:22, Simon Glass wrote:

Hi Jean-Jacques,

On 24 March 2017 at 06:24, Jean-Jacques Hiblot  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 
---
  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(_desc[0]);
+   if (init_part)
+   part_init(_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, );
+   str, IF_TYPE_SCSI,
+   -1,
+   _bd.blksz,
+   _bd.blksz * _bd.lba,
+   );

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 = 

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

[Simon Glass]

Hi Jean-Jacques,

On 24 March 2017 at 06:24, Jean-Jacques Hiblot  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 
> ---
>  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(_desc[0]);
> +   if (init_part)
> +   part_init(_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, );
> +   str, IF_TYPE_SCSI,
> +   -1,
> +   _bd.blksz,
> +   _bd.blksz * _bd.lba,
> +   );

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);
>   

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

[Jean-Jacques Hiblot]

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 
---
 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(_desc[0]);
+   if (init_part)
+   part_init(_desc[0]);
 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);
+
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, );
+   str, IF_TYPE_SCSI,
+   -1,
+   _bd.blksz,
+   _bd.blksz * _bd.lba,
+   );
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);
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, _dev_desc[scsi_max_devs]);
+   ret = scsi_detect_dev(i, _dev_desc[scsi_max_devs], 
1);
if (ret)
continue;
 
-- 
1.9.1