fix sector_div use in scsicam.c

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

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

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

Umm, right.

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

Next try:

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

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.

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

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

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

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

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.

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:

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.

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