Re: [PATCH] sanitize ->bios_param prototype

Re: [PATCH] sanitize ->bios_param prototype

[James Bottomley]

hch@lst.de said:
> (aka Scsi_Disk or Disk) to each and every lowlevel driver, although
> this structure should be privated to the sd driver.

> All bios_param implementation do only use two fields:  .device and
> .capacity.  This patch passes down those two directly and gets rid of
> 99% of the sd.h inclusions (*).

> I've tried to not break any driver with this patch, but given the
> number of compiler errors in the current tree I might have missed one
> or two. 

I'm getting:

Loading scsi_mod module
/lib/scsi_mod.o: unresolved symbol __udivdi3

The problem seems to be the definition of sector_t for the division.  If I 
make this fix:

===== drivers/scsi/scsicam.c 1.10 vs edited =====
--- 1.10/drivers/scsi/scsicam.c Thu Oct 24 19:29:22 2002
+++ edited/drivers/scsi/scsicam.c       Fri Oct 25 12:10:40 2002
@@ -80,11 +80,11 @@
        if (ret || ip[0] > 255 || ip[1] > 63) {
                ip[0] = 64;
                ip[1] = 32;
-               if ((capacity / (ip[0] * ip[1])) > 65534) {
+               if (((unsigned long)capacity / (ip[0] * ip[1])) > 65534) {
                        ip[0] = 255;
                        ip[1] = 63;
                }
-               ip[2] = capacity / (ip[0] * ip[1]);
+               ip[2] = (unsigned long)capacity / (ip[0] * ip[1]);
        }
 
        return 0;

Then everything works fine.  This is obviously not the correct fix, but it's 
as good as the previous code, so it should be OK.

James

Re: [PATCH] sanitize ->bios_param prototype

[Christoph Hellwig]

On Fri, Oct 25, 2002 at 12:18:51PM -0500, James Bottomley wrote:
> hch@lst.de said:
> > (aka Scsi_Disk or Disk) to each and every lowlevel driver, although
> > this structure should be privated to the sd driver.
> 
> > All bios_param implementation do only use two fields:  .device and
> > .capacity.  This patch passes down those two directly and gets rid of
> > 99% of the sd.h inclusions (*).
> 
> > I've tried to not break any driver with this patch, but given the
> > number of compiler errors in the current tree I might have missed one
> > or two. 
> 
> I'm getting:
> 
> Loading scsi_mod module
> /lib/scsi_mod.o: unresolved symbol __udivdi3
> 
> The problem seems to be the definition of sector_t for the division.  If I 
> make this fix:

Ooops - I tested without CONFIG_LBD defined where it's 32bit so I
couldn't trap this bug.  I think the right fix would be
to use sector_div (warning, hand-edit patch):

--- 1.10/drivers/scsi/scsicam.c Thu Oct 24 19:29:22 2002
+++ edited/drivers/scsi/scsicam.c       Fri Oct 25 12:10:40 2002
@@ -80,11 +80,12 @@
        if (ret || ip[0] > 255 || ip[1] > 63) {
                ip[0] = 64;
                ip[1] = 32;
-               if ((capacity / (ip[0] * ip[1])) > 65534) {
+               if (((unsigned long)capacity / (ip[0] * ip[1])) > 65534) {
                        ip[0] = 255;
                        ip[1] = 63;
                }
-               ip[2] = capacity / (ip[0] * ip[1]);
+         	sector_div(capacity, (ip[0] * ip[1]));      
+		ip[2] = capacity;
        }
 
        return 0;

Re: [PATCH] sanitize ->bios_param prototype

[Andries Brouwer]

On Fri, Oct 25, 2002 at 12:18:51PM -0500, James Bottomley wrote:

> Loading scsi_mod module
> /lib/scsi_mod.o: unresolved symbol __udivdi3
> 
> The problem seems to be the definition of sector_t for the division.  If I 
> make this fix:
> 
> ===== drivers/scsi/scsicam.c 1.10 vs edited =====
> --- 1.10/drivers/scsi/scsicam.c Thu Oct 24 19:29:22 2002
> +++ edited/drivers/scsi/scsicam.c       Fri Oct 25 12:10:40 2002
> @@ -80,11 +80,11 @@
>         if (ret || ip[0] > 255 || ip[1] > 63) {
>                 ip[0] = 64;
>                 ip[1] = 32;
> -               if ((capacity / (ip[0] * ip[1])) > 65534) {
> +               if (((unsigned long)capacity / (ip[0] * ip[1])) > 65534) {
>                         ip[0] = 255;
>                         ip[1] = 63;
>                 }
> -               ip[2] = capacity / (ip[0] * ip[1]);
> +               ip[2] = (unsigned long)capacity / (ip[0] * ip[1]);
>         }
>  
>         return 0;
> 
> Then everything works fine.  This is obviously not the correct fix, but it's 
> as good as the previous code, so it should be OK.

I didn't see hch's patch, but you can always write

	if (ret || ip[0] > 255 || ip[1] > 63) {
		ip[0] = 64;
		ip[1] = 32;
		if ((capacity >> 11) > 65534) {
			ip[0] = 255;
			ip[1] = 63;
		}
		if (capacity > 65535*63*255)
			ip[2] = 65535;
		else
			ip[2] = (unsigned long)capacity / (ip[0] * ip[1]);
	}

Andries


(ip[2] is returned in a short, so returning larger values is meaningless;
moreover, the current specification says that ip[2] is ignored)