Re: [hatari-devel] Patch: IDE support for sector sizes > 512 bytes

[Thomas Huth <th.huth@xxxxxxxxx>]

Am Tue, 6 Nov 2018 20:54:35 +0100
schrieb Uwe Seimet <Uwe.Seimet@xxxxxxxxx>:

> Hi Thomas,
> 
> Thank you. The attached patch contains some minor adjustments for
> optical (i.e. non hard disk) ATAPI devices.

 Hi Uwe,

some comments on this patch:

@@ -983,11 +983,11 @@
 	padstr((char *)(p + 23), FW_VERSION, 8); /* firmware version */
 	if(s == opaque_ide_if) /* model */
 	{
-		padstr((char *)(p + 27), "Hatari IDE0 disk", 40);
+		padstr((char *)(p + 27), "Hatari  IDE0 disk", 40);
 	}
 	else
 	{
-		padstr((char *)(p + 27), "Hatari IDE1 disk", 40);
+		padstr((char *)(p + 27), "Hatari  IDE1 disk", 40);
 	}

Why the white space change? Also, I think we agreed in another mail
that the name should rather not depend on the slot, so maybe we should
rather change this to "Hatari IDE disk" unconditionally?

Anyway, this should be a separate patch instead.

@@ -1802,7 +1802,6 @@
 		uint64_t total_sectors;
 
 		bdrv_get_geometry(s->bs, &total_sectors);
-		total_sectors >>= 2;
 		if (total_sectors == 0)
 		{
 			ide_atapi_cmd_error(s, SENSE_NOT_READY,

Why this change? Even current QEMU still contains the shift, so I
somehow doubt that this is really right?

@@ -1867,8 +1866,13 @@
 		ide_atapi_cmd_ok(s);
 		break;
 	case GPCMD_INQUIRY:
+	{
+		uint64_t total_sectors;
+
+		bdrv_get_geometry(s->bs, &total_sectors);
+
 		max_len = packet[4];
-		buf[0] = 0x05; /* CD-ROM */
+		buf[0] = 0x05; /* MMC drive */
 		buf[1] = 0x80; /* removable */
 		buf[2] = 0x00; /* ISO */
 		buf[3] = 0x21; /* ATAPI-2 (XXX: put ATAPI-4 ?) */
@@ -1876,11 +1880,12 @@
 		buf[5] = 0; /* reserved */
 		buf[6] = 0; /* reserved */
 		buf[7] = 0; /* reserved */
-		padstr8(buf + 8, 8, "QEMU");
-		padstr8(buf + 16, 16, "QEMU CD-ROM");
+		padstr8(buf + 8, 8, "Hatari");
+		padstr8(buf + 16, 16, "CD/DVD-ROM");
 		padstr8(buf + 32, 4, FW_VERSION);
 		ide_atapi_cmd_reply(s, 36, max_len);
 		break;
+	}

Fine for me if we change the strings here, but why do you introduce
total_sectors here without using it?

 Thomas