diff mbox

[U-Boot,v2,09/10] dm: scsi: fix divide-by-0 error in scsi_scan()

Message ID 1491565329-20267-10-git-send-email-jjhiblot@ti.com
State Accepted
Commit 4e27b9a5845e9c061b315b7ca301eff982d6898f
Delegated to: Simon Glass
Headers show

Commit Message

Jean-Jacques Hiblot April 7, 2017, 11:42 a.m. UTC
With DM_SCSI enabled, blk_create_devicef() is called with blkz = 0, leading
to a divide-by-0 exception.
scsi_detect_dev() can be used to get the required parameters (block size
and number of blocks) from the drive before calling blk_create_devicef().

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 common/scsi.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

Comments

Tom Rini April 9, 2017, 1:13 a.m. UTC | #1
On Fri, Apr 07, 2017 at 01:42:08PM +0200, Jean-Jacques Hiblot wrote:

> With DM_SCSI enabled, blk_create_devicef() is called with blkz = 0, leading
> to a divide-by-0 exception.
> scsi_detect_dev() can be used to get the required parameters (block size
> and number of blocks) from the drive before calling blk_create_devicef().
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>

Reviewed-by: Tom Rini <trini@konsulko.com>
Simon Glass April 9, 2017, 7:27 p.m. UTC | #2
Hi,

On 7 April 2017 at 05:42, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> With DM_SCSI enabled, blk_create_devicef() is called with blkz = 0, leading
> to a divide-by-0 exception.
> scsi_detect_dev() can be used to get the required parameters (block size
> and number of blocks) from the drive before calling blk_create_devicef().
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  common/scsi.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below.

>
> diff --git a/common/scsi.c b/common/scsi.c
> index 972ef338..d37222c 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -580,9 +580,19 @@ 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);
> +                               ret = scsi_detect_dev(i, lun, &_bd);
> +                               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,20 +600,27 @@ 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);
>                                 if (ret) {
>                                         debug("Can't create device\n");
>                                         return ret;
>                                 }
> -                               bdesc = dev_get_uclass_platdata(bdev);
>
> -                               scsi_init_dev_desc_priv(bdesc);
> -                               ret = scsi_detect_dev(i, lun, bdesc);
> -                               if (ret) {
> -                                       device_unbind(bdev);
> -                                       continue;
> -                               }
> +                               bdesc = dev_get_uclass_platdata(bdev);
> +                               bdesc->target = i;
> +                               bdesc->lun = lun;
> +                               bdesc->removable = _bd.removable;
> +                               bdesc->type = _bd.type;
> +                               memcpy(&bdesc->vendor, &_bd.vendor,
> +                                      sizeof(_bd.vendor));
> +                               memcpy(&bdesc->product, &_bd.product,
> +                                      sizeof(_bd.product));
> +                               memcpy(&bdesc->revision, &_bd.revision,
> +                                      sizeof(_bd.revision));

Can you please move this block (inside the double for loops) into a
separate function? It is getting too long. A follow-up patch is fine
since you have already done this.

>                                 part_init(bdesc);
>
>                                 if (mode == 1) {
> --
> 1.9.1
>

Regards,
Simon
Simon Glass April 15, 2017, 4:07 p.m. UTC | #3
On 9 April 2017 at 13:27, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 7 April 2017 at 05:42, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> With DM_SCSI enabled, blk_create_devicef() is called with blkz = 0, leading
>> to a divide-by-0 exception.
>> scsi_detect_dev() can be used to get the required parameters (block size
>> and number of blocks) from the drive before calling blk_create_devicef().
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>  common/scsi.c | 35 ++++++++++++++++++++++++++---------
>>  1 file changed, 26 insertions(+), 9 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Please see below.
>
>>
>> diff --git a/common/scsi.c b/common/scsi.c
>> index 972ef338..d37222c 100644
>> --- a/common/scsi.c
>> +++ b/common/scsi.c
>> @@ -580,9 +580,19 @@ 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);
>> +                               ret = scsi_detect_dev(i, lun, &_bd);
>> +                               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,20 +600,27 @@ 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);
>>                                 if (ret) {
>>                                         debug("Can't create device\n");
>>                                         return ret;
>>                                 }
>> -                               bdesc = dev_get_uclass_platdata(bdev);
>>
>> -                               scsi_init_dev_desc_priv(bdesc);
>> -                               ret = scsi_detect_dev(i, lun, bdesc);
>> -                               if (ret) {
>> -                                       device_unbind(bdev);
>> -                                       continue;
>> -                               }
>> +                               bdesc = dev_get_uclass_platdata(bdev);
>> +                               bdesc->target = i;
>> +                               bdesc->lun = lun;
>> +                               bdesc->removable = _bd.removable;
>> +                               bdesc->type = _bd.type;
>> +                               memcpy(&bdesc->vendor, &_bd.vendor,
>> +                                      sizeof(_bd.vendor));
>> +                               memcpy(&bdesc->product, &_bd.product,
>> +                                      sizeof(_bd.product));
>> +                               memcpy(&bdesc->revision, &_bd.revision,
>> +                                      sizeof(_bd.revision));
>
> Can you please move this block (inside the double for loops) into a
> separate function? It is getting too long. A follow-up patch is fine
> since you have already done this.
>
>>                                 part_init(bdesc);
>>
>>                                 if (mode == 1) {
>> --
>> 1.9.1
>>
>
> Regards,
> Simon

Applied to u-boot-dm, thanks!
diff mbox

Patch

diff --git a/common/scsi.c b/common/scsi.c
index 972ef338..d37222c 100644
--- a/common/scsi.c
+++ b/common/scsi.c
@@ -580,9 +580,19 @@  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);
+				ret = scsi_detect_dev(i, lun, &_bd);
+				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,20 +600,27 @@  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);
 				if (ret) {
 					debug("Can't create device\n");
 					return ret;
 				}
-				bdesc = dev_get_uclass_platdata(bdev);
 
-				scsi_init_dev_desc_priv(bdesc);
-				ret = scsi_detect_dev(i, lun, bdesc);
-				if (ret) {
-					device_unbind(bdev);
-					continue;
-				}
+				bdesc = dev_get_uclass_platdata(bdev);
+				bdesc->target = i;
+				bdesc->lun = lun;
+				bdesc->removable = _bd.removable;
+				bdesc->type = _bd.type;
+				memcpy(&bdesc->vendor, &_bd.vendor,
+				       sizeof(_bd.vendor));
+				memcpy(&bdesc->product, &_bd.product,
+				       sizeof(_bd.product));
+				memcpy(&bdesc->revision, &_bd.revision,
+				       sizeof(_bd.revision));
 				part_init(bdesc);
 
 				if (mode == 1) {