[PATCH] fix sector_div use in scsicam.c

[PATCH] fix sector_div use in scsicam.c

[Christoph Hellwig]

sector_div has the same slightly strange calling convention do_div has:
it's return value is the modulo of the two operators, the division
result is in the first parameter.  Also optimize one of the expensive
64bit division away (okay, okay - it's not exactly an fast-path :))


--- 1.11/drivers/scsi/scsicam.c	Fri Oct 25 13:31:53 2002
+++ edited/drivers/scsi/scsicam.c	Sun Oct 27 15:18:13 2002
@@ -80,11 +80,12 @@
 	if (ret || ip[0] > 255 || ip[1] > 63) {
 		ip[0] = 64;
 		ip[1] = 32;
-		if (sector_div(capacity, ip[0] * ip[1]) > 65534) {
+		sector_div(capacity, ip[0] * ip[1]);
+		if (capacity > 65534) {
 			ip[0] = 255;
 			ip[1] = 63;
 		}
-		ip[2] = sector_div(capacity, ip[0] * ip[1]);
+		ip[2] = capacity;
 	}
 
 	return 0;

Re: [PATCH] fix sector_div use in scsicam.c

[James Bottomley]

hch@lst.de said:
> sector_div has the same slightly strange calling convention do_div
> has: it's return value is the modulo of the two operators, the
> division result is in the first parameter.  Also optimize one of the
> expensive 64bit division away (okay, okay - it's not exactly an
> fast-path :)) 

Oops, I thought the semantics were the other way around...

> -		ip[2] = sector_div(capacity, ip[0] * ip[1]);
> +		ip[2] = capacity; 

This doesn't look right.  We updated the divisors (ip[0] and ip[1]) in the if 
statement, so surely we have to recalculate the division?

James

Re: [PATCH] fix sector_div use in scsicam.c

[Christoph Hellwig]

On Sun, Oct 27, 2002 at 10:21:44AM -0600, James Bottomley wrote:
> hch@lst.de said:
> > sector_div has the same slightly strange calling convention do_div
> > has: it's return value is the modulo of the two operators, the
> > division result is in the first parameter.  Also optimize one of the
> > expensive 64bit division away (okay, okay - it's not exactly an
> > fast-path :)) 
> 
> Oops, I thought the semantics were the other way around...
> 
> > -		ip[2] = sector_div(capacity, ip[0] * ip[1]);
> > +		ip[2] = capacity; 
> 
> This doesn't look right.  We updated the divisors (ip[0] and ip[1]) in the if 
> statement, so surely we have to recalculate the division?

Umm, right.


--- 1.11/drivers/scsi/scsicam.c	Fri Oct 25 13:31:53 2002
+++ edited/drivers/scsi/scsicam.c	Sun Oct 27 15:18:13 2002
@@ -80,11 +80,13 @@
 	if (ret || ip[0] > 255 || ip[1] > 63) {
 		ip[0] = 64;
 		ip[1] = 32;
-		if (sector_div(capacity, ip[0] * ip[1]) > 65534) {
+		sector_div(capacity, ip[0] * ip[1]);
+		if (capacity > 65534) {
 			ip[0] = 255;
 			ip[1] = 63;
 		}
-		ip[2] = sector_div(capacity, ip[0] * ip[1]);
+		sector_div(capacity, ip[0] * ip[1]);
+		ip[2] = capacity;
 	}
 
 	return 0;

Re: [PATCH] fix sector_div use in scsicam.c

[James Bottomley]

> -		ip[2] = sector_div(capacity, ip[0] * ip[1]);
> +		sector_div(capacity, ip[0] * ip[1]);
> +		ip[2] = capacity;

Now you've lost your optimisation.  The second sector_div should be inside the 
if.

James

Re: [PATCH] fix sector_div use in scsicam.c

[Christoph Hellwig]

On Sun, Oct 27, 2002 at 10:34:09AM -0600, James Bottomley wrote:
> > -		ip[2] = sector_div(capacity, ip[0] * ip[1]);
> > +		sector_div(capacity, ip[0] * ip[1]);
> > +		ip[2] = capacity;
> 
> Now you've lost your optimisation.  The second sector_div should be inside the 
> if.

Next try:


--- 1.11/drivers/scsi/scsicam.c	Fri Oct 25 13:31:53 2002
+++ edited/drivers/scsi/scsicam.c	Sun Oct 27 16:36:31 2002
@@ -80,11 +80,13 @@
 	if (ret || ip[0] > 255 || ip[1] > 63) {
 		ip[0] = 64;
 		ip[1] = 32;
-		if (sector_div(capacity, ip[0] * ip[1]) > 65534) {
+		sector_div(capacity, ip[0] * ip[1]);
+		if (capacity > 65534) {
 			ip[0] = 255;
 			ip[1] = 63;
+			sector_div(capacity, ip[0] * ip[1]);
 		}
-		ip[2] = sector_div(capacity, ip[0] * ip[1]);
+		ip[2] = capacity;
 	}
 
 	return 0;

Re: [PATCH] fix sector_div use in scsicam.c

[Andries Brouwer]

On Sun, Oct 27, 2002 at 05:37:56PM +0100, Christoph Hellwig wrote:
> On Sun, Oct 27, 2002 at 10:34:09AM -0600, James Bottomley wrote:

Hi Christoph & James,

I see you trying to get this scsicam code do something
that it should not. No 64-bit handling required or even desirable.
Any use of sector_div here is wrong.

I answered this yesterday or the day before or so, but maybe
you did not see the reply.

> --- 1.11/drivers/scsi/scsicam.c	Fri Oct 25 13:31:53 2002
> +++ edited/drivers/scsi/scsicam.c	Sun Oct 27 16:36:31 2002
> @@ -80,11 +80,13 @@
>  	if (ret || ip[0] > 255 || ip[1] > 63) {
>  		ip[0] = 64;
>  		ip[1] = 32;
> -		if (sector_div(capacity, ip[0] * ip[1]) > 65534) {
> +		sector_div(capacity, ip[0] * ip[1]);
> +		if (capacity > 65534) {
>  			ip[0] = 255;
>  			ip[1] = 63;
> +			sector_div(capacity, ip[0] * ip[1]);
>  		}
> -		ip[2] = sector_div(capacity, ip[0] * ip[1]);
> +		ip[2] = capacity;
>  	}
>  
>  	return 0;

Write, e.g.,

	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

Re: [PATCH] fix sector_div use in scsicam.c

[James Bottomley]

aebr@win.tue.nl said:
> I see you trying to get this scsicam code do something that it should
> not. No 64-bit handling required or even desirable. Any use of
> sector_div here is wrong. 

Could you explain why?  Once the disc is too big for the HDIO_GETGEO 
interface, we're returning rubbish anyway.  Whether we overflow or truncate 
seems irrelevant.  The ide code overflows, it doesn't truncate to the maximum, 
so we're at least following existing conventions.

However, should we ever implement the big_geometry interface, then we would 
still be able to use the routine as Christoph has coded it, so the fix looks 
like the correct future proofing to me.

If you really want the truncation, it should probably be done in the sd_ioctl 
code.

James

Re: [PATCH] fix sector_div use in scsicam.c

[Andries Brouwer]

On Sun, Oct 27, 2002 at 02:53:29PM -0600, James Bottomley wrote:
> aebr@win.tue.nl said:
> > I see you trying to get this scsicam code do something that it should
> > not. No 64-bit handling required or even desirable. Any use of
> > sector_div here is wrong. 
> 
> Could you explain why?

On ancient machines, user space needs the number of sectors per track
and the number of heads in order to (i) write something appropriate
in the partition table, and (ii) help the boot loader interface with
the BIOS at boot time.

However, the number of cylinders does not play a role.
On new systems the geometry is entirely irrelevant.
On old systems the number of cylinders is not needed anywhere.
It is not written in the partition table. It is not used in BIOS interaction.

Some ancient software used C*H*S as a measure for the total disk size,
but that is broken, especially since geometry depends on the BIOS setup.
These days it is especially broken since HDIO_GETGEO returns a 16-bit
value for the number of cylinders.

The code I suggested returns something reasonable to be backwards
compatible with ancient software. Without the desire for compatibility
the assignment
	ip[2] = 0;
would be acceptable too.

The patches I reacted to were worse in two respects:
It invoked the sector_div macro designed to divide large numbers,
while here division of large numbers is meaningless.
Just superfluous code.
It suggests that the computation is meaningful, and before you know it
people will add ioctls to get the bits computed by this division
out to user space.

The current spec for HDIO_GETGEO is that the cylinder field of
what it returns has to be ignored. The total disk size is found
using BLKGETSIZE. If user space wants to compute a number of
cylinders, for whatever reason, it has to divide the total size
by H*S as returned by HDIO_GETGEO. That is what all recent software
does.

I am in the process of removing, extremely slowly, everything related
to disk geometries from the kernel. Disk geometries have not existed
the past ten years, and thousands of users have had to fight fake
geometries invented by disk, BIOS, kernel and software.
We must not add to this cruft, but only take away. Slowly.

Andries

Re: [PATCH] fix sector_div use in scsicam.c

[James Bottomley]

If the return type will be ignored by most applications, I don't see what the 
problem is.

I'm all for ripping the C/H/S rubbish out of the kernel, just give me the 
patches when you have them.  Until that time, we need the changes Christoph 
has made now to solve a layering violation in SCSI.  And, since I never want 
to see the bios_param stuff again, I'm not going to put in code (like an 
obviously wrong truncation) that would attract attention in a large block size 
audit of the SCSI subsystem.  The sector_div is an obviously correct thing to 
do for a sector_t quantity, so it's staying.

James

Re: [PATCH] fix sector_div use in scsicam.c

[Andries Brouwer]

On Sun, Oct 27, 2002 at 06:05:07PM -0600, James Bottomley wrote:

> If the return type will be ignored by most applications, I don't see
> what the problem is.

There is no problem. My longish reaction was mostly because you used
"future proofing", that gave the impression that you did not know
what this is about.

> (like an obviously wrong truncation)

No, the code I wrote was optimal.
If you have 16 bits and the value is 70000, I prefer returning
65535 over 4464.

Remember: this code has one and only one function: to compute
the 16-bit value that HDIO_GETGEO is going to return.

For modern software this does not matter in the least.
For ancient stuff, in the case of overflow, my patch gives the disk
a size of 539 GB. The patch I objected to gives a random size.

Andries

By the way: quite apart from the fact that sector_div is
superfluous, it is also a very obscure and nonintuitive function.
We can see that from the fact that several incorrect versions
were proposed already. And the latest one I saw:

> --- 1.11/drivers/scsi/scsicam.c       Fri Oct 25 13:31:53 2002
> +++ edited/drivers/scsi/scsicam.c     Sun Oct 27 16:36:31 2002
> @@ -80,11 +80,13 @@
>       if (ret || ip[0] > 255 || ip[1] > 63) {
>               ip[0] = 64;
>               ip[1] = 32;
> -             if (sector_div(capacity, ip[0] * ip[1]) > 65534) {
> +             sector_div(capacity, ip[0] * ip[1]);
> +             if (capacity > 65534) {
>                       ip[0] = 255;
>                       ip[1] = 63;
> +                     sector_div(capacity, ip[0] * ip[1]);
>               }
> -             ip[2] = sector_div(capacity, ip[0] * ip[1]);
> +             ip[2] = capacity;
>       }
>  
>       return 0;

is also incorrect: it divides capacity twice.

Re: [PATCH] fix sector_div use in scsicam.c

[Christoph Hellwig]

On Mon, Oct 28, 2002 at 01:50:53AM +0100, Andries Brouwer wrote:
> On Sun, Oct 27, 2002 at 06:05:07PM -0600, James Bottomley wrote:
> 
> > If the return type will be ignored by most applications, I don't see
> > what the problem is.
> 
> There is no problem. My longish reaction was mostly because you used
> "future proofing", that gave the impression that you did not know
> what this is about.
> 
> > (like an obviously wrong truncation)
> 
> No, the code I wrote was optimal.
> If you have 16 bits and the value is 70000, I prefer returning
> 65535 over 4464.

Why didn't you write is as patch?  James, this in Andries code
in patch from, it get's rid of the ugly sector_div so it should
be in if not just because of that.

--- 1.12/drivers/scsi/scsicam.c	Sun Oct 27 11:56:37 2002
+++ edited/drivers/scsi/scsicam.c	Mon Oct 28 01:12:24 2002
@@ -78,15 +78,18 @@
 	/* if something went wrong, then apparently we have to return
 	   a geometry with more than 1024 cylinders */
 	if (ret || ip[0] > 255 || ip[1] > 63) {
-		ip[0] = 64;
-		ip[1] = 32;
-		sector_div(capacity, ip[0] * ip[1]);
-		if (capacity > 65534) {
+		if ((capacity >> 11) > 65534) {
 			ip[0] = 255;
 			ip[1] = 63;
-			sector_div(capacity, ip[0] * ip[1]);
+		} else {
+			ip[0] = 64;
+			ip[1] = 32;
 		}
-		ip[2] = capacity;
+
+		if (capacity > 65535*63*255)
+			ip[2] = 65535;
+		else
+			ip[2] = (unsigned long)capacity / (ip[0] * ip[1]);
 	}
 
 	return 0;