Re: [parisc-linux] Floppy disk support for 9000 series (715, 755, etc.) workstations.

Re: [parisc-linux] Floppy disk support for 9000 series (715, 755, etc.) workstations.

[Grant Grundler]

On Tue, Jan 23, 2007 at 03:51:32PM -0500, James K. Love wrote:
> All:
> 
> I posted this to debian-hppa, but I thought I'd pass this along to
> parisc-linux in case anyone isn't a member of both lists. If you are,
> I apologize in advance for cluttering you inbox...
> 
> Could someone provide some details about why the TEAC floppy disk drives
> (scsi) in the 9000 series workstations are not supported in the parisc
> port?

At one point I thought they did work.

>   Is it due to a lack of documentation, missing sd/53c700 support for
> floppy drives, or lack of interest in floppy devices in general (or some
> combination of the three)?

THere is clearly a lack of interest in the non-scsi floppy.
I've handed out some SCSI floppy drives in the past.

>   I realize floppy drives are antiquated and
> scsi floppy drives are an enigma, but the application I'd like to run on
> these machines requires floppy support, which worked fine under HP-UX. 
> In an attempt investigate further, I installed MKLinux on a 715, and the
> scsi floppy drive was supported through some combination of the MACH
> kernel and it's hosted Linux kernel.  I've looked through the MKLinux
> source code, and it appears that the MACH scsi driver had some support
> for flexible disk drives (which I assume is the floppy drive), but I'm
> uncertain if this is the only missing piece to the puzzle.  If anyone
> has any additional info/insight, I'd really appreciated it.

If you can figure out what's missing between linux and MKlinux there
is a good change it could get added. The closer you get to providing
a patch, the more likely it will get included.

hth,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Floppy disk support for 9000 series (715, 755, etc.) workstations.

[James K. Love]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]

On 1/17/2007 2:16 PM, Matt Taggart wrote:
> "James Love" writes...
>> They use a HP-proprietary TEAC SCSI floppy disk, which appears to be an
>> IDE drive with a SCSI-IO/FDC board piggy-backed on it. Any idea whether
>> the TEAC drive is the one that HP won't provide the documents for?
>
> I hope you are able to make progress on a driver, that would be cool. But I
> don't think anyone from HP is going to be able to help. :(
>
>> Regardless, the drive works in MKLinux, so hopefully someone knows
>> something about how it works...
>
> Yeah it sounds like there is hope of getting it working then.
>

On 1/24/2007 11:28 AM, Grant Grundler wrote:
> On Tue, Jan 23, 2007 at 03:51:32PM -0500, James K. Love wrote:
>> In an attempt investigate further, I installed MKLinux on a 715, and the
>> scsi floppy drive was supported through some combination of the MACH
>> kernel and it's hosted Linux kernel.  I've looked through the MKLinux
>> source code, and it appears that the MACH scsi driver had some support
>> for flexible disk drives (which I assume is the floppy drive), but I'm
>> uncertain if this is the only missing piece to the puzzle.  If anyone
>> has any additional info/insight, I'd really appreciated it.
> 
> If you can figure out what's missing between linux and MKlinux there
> is a good change it could get added. The closer you get to providing
> a patch, the more likely it will get included.
> 

I finally figured some of this out.  I ported the MACH flexible scsi disk
code over to the 2.6 kernel, and I can now mount/read/write with my HP TEAC FC-1
drive.  The modifications I made were inside the scsi device (sd) driver, which
is similar to where they were inside the MACH kernel.  This is obviously not
ideal, since the device major is for a scsi disk, but the drive is really a
floppy.  This breaks some user-space floppy apps like fdformat.  There really
should be a new floppy driver similar to the cdrom driver, but these changes may
not work with any other scsi floppy drive (are there any others?).  I only have
a few HP OEM TEAC revs to test with here.

Bottom line... The scsi disk driver could be patched, but it may only provide
partial/experimental support for this drive without extensive changes.  I'm
traditionally a user-space developer, so I'd appreciate any advice/thoughts
before I get knee deep.  Also, if someone could pass along a few links to some
procedural docs about properly submitting kernel code/patches, I'd really
appreciate it.

Thanks,
James L.






















[-- Attachment #2: jlove.vcf --]
[-- Type: text/x-vcard, Size: 354 bytes --]

begin:vcard
fn:James K. Love
n:Love;James K.
org:SPAWAR Systems Center Charleston, Code 523;Scientific Research Corporation, ISS Division
adr:Building 3452, WS-20;;One Innovation Drive;Hanahan;SC;29406;USA
email;internet:jlove@scires.com
title:Software Engineer
tel;work:843.218.6532
x-mozilla-html:FALSE
url:http://www.scires.com
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Floppy disk support for 9000 series (715, 755, etc.) workstations.

[Matthew Wilcox]

On Thu, Feb 08, 2007 at 12:23:41PM -0500, James K. Love wrote:
> I finally figured some of this out.  I ported the MACH flexible scsi disk
> code over to the 2.6 kernel, and I can now mount/read/write with my HP TEAC 
> FC-1
> drive.  The modifications I made were inside the scsi device (sd) driver, 
> which
> is similar to where they were inside the MACH kernel.  This is obviously not
> ideal, since the device major is for a scsi disk, but the drive is really a
> floppy.  This breaks some user-space floppy apps like fdformat.  There 
> really
> should be a new floppy driver similar to the cdrom driver, but these 
> changes may
> not work with any other scsi floppy drive (are there any others?).  I only 
> have
> a few HP OEM TEAC revs to test with here.
> 
> Bottom line... The scsi disk driver could be patched, but it may only 
> provide
> partial/experimental support for this drive without extensive changes.  I'm
> traditionally a user-space developer, so I'd appreciate any advice/thoughts
> before I get knee deep.  Also, if someone could pass along a few links to 
> some
> procedural docs about properly submitting kernel code/patches, I'd really
> appreciate it.

That's good work!

Could you post the patch to sd.c here?  I, and others, can look it over,
critique it, and help polish it before you submit it to the linux-scsi
list.  Or if you're brave, you could send it straight to linux-scsi
(there's about 3 of us who're on both lists, so you may well see feedback
>from a familiar name anyway).

Without looking at the source code to check, I suspect the fdformat code
is trying ioctls that are implemented in floppy.c and we might want to
emulate them in sd.c if the device is a floppy.

We certainly could do a sf.c that's similar in spirit to sr, sd, st, etc.
That might be the right approach, or it might be best to extend sd.
The SCSI-2 spec says that floppy drives are Direct Access Devices (which
sd normally drives).  But then, if we want to make it appear to Linux as
if it's a floppy drive, sf.c would be the right way to go.  If we want
to support machines with both a scsi floppy and a regular floppy at the
same time (and such machines can certainly be constructed; eg a C3000
with a SuperIO floppy and a scsi floppy, or a 725 with a Lasi floppy and
a scsi floppy), then I think we have to use the 'sd' approach to avoid
the drivers fighting over the major number.  A disadvantage to that is
the lack of different device nodes for different sector sizes ... otoh,
some may see that as an advantage ;-)

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Floppy disk support for 9000 series (715, 755, etc.) workstations.

[James K. Love]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On 2/8/2007 12:38 PM, Matthew Wilcox wrote:
> Could you post the patch to sd.c here?  I, and others, can look it over,
> critique it, and help polish it before you submit it to the linux-scsi
> list.  Or if you're brave, you could send it straight to linux-scsi
> (there's about 3 of us who're on both lists, so you may well see feedback
> from a familiar name anyway).

Sure.  I'm working on this for a project that is running Sarge with the 2.6.8
kernel, so I'll need to migrate the changes over to the latest sd.c from the
parisc-2.6 tree, then clean it up a bit.  I'll post a patch as soon as its ready
for general consumption.

> Without looking at the source code to check, I suspect the fdformat code
> is trying ioctls that are implemented in floppy.c and we might want to
> emulate them in sd.c if the device is a floppy.

I haven't looked at the fdformat code either, but it aborts early due to the
scsi floppy device having the wrong major number.  I don't think it even gets to
any ioctls.  BTW, the mtools functions all appear to work.





















[-- Attachment #2: jlove.vcf --]
[-- Type: text/x-vcard, Size: 354 bytes --]

begin:vcard
fn:James K. Love
n:Love;James K.
org:SPAWAR Systems Center Charleston, Code 523;Scientific Research Corporation, ISS Division
adr:Building 3452, WS-20;;One Innovation Drive;Hanahan;SC;29406;USA
email;internet:jlove@scires.com
title:Software Engineer
tel;work:843.218.6532
x-mozilla-html:FALSE
url:http://www.scires.com
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 169 bytes --]

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] Floppy disk support for 9000 series (715, 755, etc.) workstations.

[Matthew Wilcox]

On Thu, Feb 08, 2007 at 03:08:55PM -0500, James K. Love wrote:
> I haven't looked at the fdformat code either, but it aborts early due to the
> scsi floppy device having the wrong major number.  I don't think it even 
> gets to
> any ioctls.  BTW, the mtools functions all appear to work.

Oh ... I guess we just need to fix fdformat then.  Thanks.
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

[parisc-linux] [PATCH] sd: Adds flexible disk (TEAC FC-1, scsi-floppy) support to scsi disk driver

[James K. Love]

All:

I've just merged my flexible disk changes (see thread link below) into the
latest scsi disk driver source.  These changes add scsi-floppy disk support
(particularly HP's revs of the TEAC FC-1 drive) to the scsi disk driver.  Per
Matthew's suggestion, I'm cowardly posting here first to get some feedback,
rather than posting straight to linux-scsi.  BTW, I will also provide a patch
for Debian Sarge, if anyone out there is interested in that too.  My patch
submittal email is appended below.

James

http://lists.parisc-linux.org/pipermail/parisc-linux/2007-February/031188.html

---

This patch adds flexible disk support for the TEAC FC-1 scsi-floppy drive to the
scsi disk driver (sd.c).  OEM versions (FD-235HS715, etc.) of the FC-1 are
prevalent in older HP parisc workstations.  This patch may also allow similar
scsi-floppy drives to work on other platforms, but I have not tested this on
anything other than HP OEM TEAC FC-1 drives.

Signed-off-by: James K. Love <jlove@scires.com>

---

This patch is essentially a port of the MACH scsi driver's flexible disk bits to
the latest 2.6 linux kernel, which provides rudimentary scsi-floppy support for
drives in older HP parisc workstations.  The changes have been tested with the
TEAC FC-1 on several older HP parisc boxes (models 715 & 755).  Some userspace
floppy apps may also need to be made scsi-floppy aware.  For instance, fdformat
is broken.  I'm using udev to create a sym-link between the scsi device and fd0.
 With the link in place, the mtools functions work well and mformat can be used
to msdos format scsi floppies.  This diff was created against the latest in the
parisc-2.6 git tree (2.6.20) and was also tested against Linus' latest linux-2.6
git tree (2.6.21-rc2).

---

--- ./drivers/scsi/sd.orig	2007-02-25 22:46:56.000000000 -0500
+++ ./drivers/scsi/sd.c	2007-03-04 21:26:00.000000000 -0500
@@ -49,6 +49,7 @@
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <asm/uaccess.h>
+#include <asm/byteorder.h>

 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
@@ -275,6 +276,79 @@ static struct scsi_driver sd_template =
 	.issue_flush		= sd_issue_flush,
 };

+/* store the scsi-floppy geometry info */
+static unsigned int sf_heads=0, sf_sectors=0, sf_cylinders=0;
+
+/*
+ * This struct was taken directly from the MACH micro-kernel scsi
+ * driver source with only minor modification.
+ * Page 5 - flexible disk drive geometry page structure.
+ */
+typedef struct {
+#ifdef __BIG_ENDIAN_BITFIELD
+	unsigned char   ps        : 1;
+	unsigned char   reserved1 : 1;
+	unsigned char   page_code : 6;
+#else
+	unsigned char   page_code : 6;
+	unsigned char   reserved1 : 1;
+	unsigned char   ps        : 1;
+#endif
+	unsigned char   page_length;
+	unsigned char   transfer_rate_msb;
+	unsigned char   transfer_rate_lsb;
+	unsigned char   number_of_heads;
+	unsigned char   sectors_per_track;
+	unsigned char   bytes_per_sector_msb;
+	unsigned char   bytes_per_sector_lsb;
+	unsigned char   number_of_cylinders_msb;
+	unsigned char   number_of_cylinders_lsb;
+	unsigned char   starting_cyl_wpc_msb;
+	unsigned char   starting_cyl_wpc_lsb;
+	unsigned char   starting_cyl_rwc_msb;
+	unsigned char   starting_cyl_rwc_lsb;
+	unsigned char   drive_step_rate_msb;
+	unsigned char   drive_step_rate_lsb;
+	unsigned char   drive_step_pule_width;
+	unsigned char   head_settle_delay_msb;
+	unsigned char   head_settle_delay_lsb;
+	unsigned char   motor_on_delay;
+	unsigned char   motor_off_delay;
+#ifdef __BIG_ENDIAN_BITFIELD
+	unsigned char   true_rdy            : 1;
+	unsigned char   start_sector_number : 1;
+	unsigned char   motor_on            : 1;
+	unsigned char   reserved2           : 5;
+	unsigned char   reserved3           : 4;
+	unsigned char   step_pulse_per_cyl  : 4;
+#else
+	unsigned char   reserved2           : 5;
+	unsigned char   motor_on            : 1;
+	unsigned char   start_sector_number : 1;
+	unsigned char   true_rdy            : 1;
+	unsigned char   step_pulse_per_cyl  : 4;
+	unsigned char   reserved3           : 4;
+#endif
+	unsigned char   write_precomp_value;
+	unsigned char   head_load_delay;
+	unsigned char   head_unload_delay;
+#ifdef __BIG_ENDIAN_BITFIELD
+	unsigned char   pin_34              : 4;
+	unsigned char   pin_2               : 4;
+	unsigned char   pin_4               : 4;
+	unsigned char   pin_1               : 4;
+#else
+	unsigned char   pin_2               : 4;
+	unsigned char   pin_34              : 4;
+	unsigned char   pin_1               : 4;
+	unsigned char   pin_4               : 4;
+#endif
+	unsigned char   reserved4;
+	unsigned char   reserved5;
+	unsigned char   reserved6;
+	unsigned char   reserved7;
+} scsi_mode_sense_page5_t  __attribute__ ((packed));
+
 /*
  * Device no to disk mapping:
  *
@@ -642,6 +716,14 @@ static int sd_getgeo(struct block_device
 	struct Scsi_Host *host = sdp->host;
 	int diskinfo[4];

+	/* If this is a scsi floppy */
+	if (sdp->removable && (sdp->type != TYPE_MOD)) {
+		geo->heads = sf_heads;
+		geo->sectors = sf_sectors;
+		geo->cylinders = sf_cylinders;
+		return 0;
+	}
+
 	/* default to most commonly used values */
         diskinfo[0] = 0x40;	/* 1 << 6 */
        	diskinfo[1] = 0x20;	/* 1 << 5 */
@@ -1011,12 +1093,21 @@ static int media_not_present(struct scsi

 	if (!scsi_sense_valid(sshdr))
 		return 0;
-	/* not invoked for commands that could return deferred errors */
-	if (sshdr->sense_key != NOT_READY &&
-	    sshdr->sense_key != UNIT_ATTENTION)
-		return 0;
-	if (sshdr->asc != 0x3A) /* medium not present */
-		return 0;
+
+	/* If this is a scsi floppy */
+	if (sdkp->device->removable && (sdkp->device->type != TYPE_MOD)) {
+		if ((sshdr->sense_key != NOT_READY)
+		    && (sshdr->asc != 0x4)) {	/* no scsi-floppy media */
+			return 0;
+		}
+	} else {
+		/* not invoked for commands that could return deferred errors */
+		if (sshdr->sense_key != NOT_READY &&
+		    sshdr->sense_key != UNIT_ATTENTION)
+			return 0;
+		if (sshdr->asc != 0x3A) /* medium not present */
+			return 0;
+	}

 	set_media_not_present(sdkp);
 	return 1;
@@ -1535,6 +1626,11 @@ static int sd_revalidate_disk(struct gen
 	struct scsi_device *sdp = sdkp->device;
 	unsigned char *buffer;
 	unsigned ordered;
+	char mode_buf[0xFF];
+	scsi_mode_sense_page5_t* page5 = NULL;
+	struct scsi_mode_data data;
+	int length = 0;
+	int res = 0;

 	SCSI_LOG_HLQUEUE(3, printk("sd_revalidate_disk: disk=%s\n", disk->disk_name));

@@ -1563,6 +1659,66 @@ static int sd_revalidate_disk(struct gen
 	sd_spinup_disk(sdkp, disk->disk_name);

 	/*
+	 * Assuming here that this check will suffice for ID'ing a scsi floppy.
+	 * Note that sd_spinup_disk sets media_present above.
+	 */
+	if (sdp->removable && (sdp->type != TYPE_MOD)
+	                   && sdkp->media_present) {
+		memset(&data, 0, sizeof(data));
+		memset(&mode_buf, 0, sizeof(mode_buf));
+
+		/* request the page 5 'flexible' sense data */
+		res = sd_do_mode_sense(sdp, 0, 5, mode_buf, 0xFF, &data, NULL);
+
+		if (!scsi_status_is_good(res)) {
+			printk(KERN_WARNING "(sd_revalidate_disk:) Flexible disk mode"
+			       " sense failure.\n");
+			goto out;
+		}
+
+		length = data.length - data.header_length - data.block_descriptor_length;
+		page5 = (scsi_mode_sense_page5_t *)(mode_buf + data.header_length
+		                                             + data.block_descriptor_length);
+
+		/* set sector size to 512 bytes, double-density media */
+		data.medium_type = 2;
+		data.device_specific &= ~0x90;
+		data.block_descriptor_length = 0;
+
+		page5->ps = 0;
+		page5->page_code &= ~0x80;
+		page5->sectors_per_track = page5->sectors_per_track *
+		                           (page5->bytes_per_sector_msb << 8 |
+		                           page5->bytes_per_sector_lsb) / 512;
+
+		page5->bytes_per_sector_msb = 2;
+		page5->bytes_per_sector_lsb = 0;
+
+		/* mode select to set geometry */
+                if (scsi_mode_select(sdp, 1, 0, 5, (char *)page5, length,
+		                     SD_TIMEOUT, SD_MAX_RETRIES, &data, NULL)) {
+			printk(KERN_WARNING "(sd_revalidate_disk:) Flexible disk mode"
+			       " select failure.\n");
+			goto out;
+		}
+
+		/* read newly selected Page 5 sense data */
+		res = sd_do_mode_sense(sdp, 0, 5, mode_buf, 0xFF, &data, NULL);
+
+		if (!scsi_status_is_good(res)) {
+			printk(KERN_WARNING "(sd_revalidate_disk:) Flexible disk mode"
+			       " sense failure.\n");
+			goto out;
+		}
+
+		/* store the drive's geometry info we just read */
+		sf_heads = page5->number_of_heads;
+		sf_sectors = page5->sectors_per_track;
+		sf_cylinders = page5->number_of_cylinders_msb << 8
+		               | page5->number_of_cylinders_lsb;
+	}
+
+	/*
 	 * Without media there is no reason to ask; moreover, some devices
 	 * react badly if we do.
 	 */


_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] sd: Adds flexible disk (TEAC FC-1, scsi-floppy) support to scsi disk driver

[Grant Grundler]

On Mon, Mar 05, 2007 at 05:17:38PM -0500, James K. Love wrote:
> All:
> 
> I've just merged my flexible disk changes (see thread link below) into the
> latest scsi disk driver source.  These changes add scsi-floppy disk support
> (particularly HP's revs of the TEAC FC-1 drive) to the scsi disk driver.  Per
> Matthew's suggestion, I'm cowardly posting here first to get some feedback,
> rather than posting straight to linux-scsi.

That's fine. In general, very nice work!
I only have some some coding style issues and one question.

> --- ./drivers/scsi/sd.orig	2007-02-25 22:46:56.000000000 -0500
> +++ ./drivers/scsi/sd.c	2007-03-04 21:26:00.000000000 -0500
...
> +#ifdef __BIG_ENDIAN_BITFIELD
> +	unsigned char   true_rdy            : 1;
> +	unsigned char   start_sector_number : 1;
> +	unsigned char   motor_on            : 1;
> +	unsigned char   reserved2           : 5;
> +	unsigned char   reserved3           : 4;
> +	unsigned char   step_pulse_per_cyl  : 4;
> +#else
> +	unsigned char   reserved2           : 5;
> +	unsigned char   motor_on            : 1;
> +	unsigned char   start_sector_number : 1;
> +	unsigned char   true_rdy            : 1;
> +	unsigned char   step_pulse_per_cyl  : 4;
> +	unsigned char   reserved3           : 4;
> +#endif

I prefer bit masks over bit fields for two reasons:
1) don't have the endian mess
2) assignment to bit fields are NOT atomic - they are read/modify/write ops.
   Use of bit fields hides this behavior.

But if you prefer them, I don't think their use is prohibited in general.

...
> @@ -1535,6 +1626,11 @@ static int sd_revalidate_disk(struct gen
>  	struct scsi_device *sdp = sdkp->device;
>  	unsigned char *buffer;
>  	unsigned ordered;
> +	char mode_buf[0xFF];
> +	scsi_mode_sense_page5_t* page5 = NULL;
> +	struct scsi_mode_data data;
> +	int length = 0;
> +	int res = 0;

Could these new local vars be moved into the code block inside
the "if ()" statement where they are used?

Just makes it easier to understand variable usage when reviewing
the code in the future.

>  	SCSI_LOG_HLQUEUE(3, printk("sd_revalidate_disk: disk=%s\n", disk->disk_name));
> 
> @@ -1563,6 +1659,66 @@ static int sd_revalidate_disk(struct gen
>  	sd_spinup_disk(sdkp, disk->disk_name);
> 
>  	/*
> +	 * Assuming here that this check will suffice for ID'ing a scsi floppy.
> +	 * Note that sd_spinup_disk sets media_present above.
> +	 */
> +	if (sdp->removable && (sdp->type != TYPE_MOD)
> +	                   && sdkp->media_present) {
> +		memset(&data, 0, sizeof(data));
> +		memset(&mode_buf, 0, sizeof(mode_buf));
...

This code chunk is long enough that it should be it's own subroutine.
Would that be difficult?

...
> +		/* store the drive's geometry info we just read */
> +		sf_heads = page5->number_of_heads;
> +		sf_sectors = page5->sectors_per_track;
> +		sf_cylinders = page5->number_of_cylinders_msb << 8
> +		               | page5->number_of_cylinders_lsb;

I'm kinda nervous about the use of static globals to "return" the
result of the previous code. In case some whacko decided to
have two scsi floppies with different media types, would this break?

Can't we directly park this info into the per disk data struct?

thanks,
grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] sd: Adds flexible disk (TEAC FC-1, scsi-floppy) support to scsi disk driver

[James K. Love]

Thanks for the comments, Grant.

On 3/8/2007 1:53 AM, Grant Grundler wrote:
> ...
>> @@ -1535,6 +1626,11 @@ static int sd_revalidate_disk(struct gen
>>  	struct scsi_device *sdp = sdkp->device;
>>  	unsigned char *buffer;
>>  	unsigned ordered;
>> +	char mode_buf[0xFF];
>> +	scsi_mode_sense_page5_t* page5 = NULL;
>> +	struct scsi_mode_data data;
>> +	int length = 0;
>> +	int res = 0;
> 
> Could these new local vars be moved into the code block inside
> the "if ()" statement where they are used?

Yes.  Helge emailed me off-list pointing this out too.

>> @@ -1563,6 +1659,66 @@ static int sd_revalidate_disk(struct gen
>>  	sd_spinup_disk(sdkp, disk->disk_name);
>>
>>  	/*
>> +	 * Assuming here that this check will suffice for ID'ing a scsi floppy.
>> +	 * Note that sd_spinup_disk sets media_present above.
>> +	 */
>> +	if (sdp->removable && (sdp->type != TYPE_MOD)
>> +	                   && sdkp->media_present) {
>> +		memset(&data, 0, sizeof(data));
>> +		memset(&mode_buf, 0, sizeof(mode_buf));
> ...
> 
> This code chunk is long enough that it should be it's own subroutine.
> Would that be difficult?

Not at all.  I didn't do too much beautification before I sent this, and I agree
it should be.

> ...
>> +		/* store the drive's geometry info we just read */
>> +		sf_heads = page5->number_of_heads;
>> +		sf_sectors = page5->sectors_per_track;
>> +		sf_cylinders = page5->number_of_cylinders_msb << 8
>> +		               | page5->number_of_cylinders_lsb;
> 
> I'm kinda nervous about the use of static globals to "return" the
> result of the previous code. In case some whacko decided to
> have two scsi floppies with different media types, would this break?

I also agree.  In retrospect, I shouldn't have done this the way I did.  I
mentioned this to Helge earlier, and I've already changed these to static consts
in my source, since I'm basically hard-coding the FC-1's geometry and consts
allow me to sanity check the mode select that sets the geometry.  Does that
sound logical?  Technically, it wouldn't have broken the way it is written
because the geometry should always be the same for these drives, but it wasn't
the best implementation.

I'll wait a bit longer to see if I get any more comments, then I'll post an
updated patch that will include your suggestions.

Regards,
James

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] sd: Adds flexible disk (TEAC FC-1, scsi-floppy) support to scsi disk driver

[Grant Grundler]

On Thu, Mar 08, 2007 at 10:04:42AM -0500, James K. Love wrote:
> Thanks for the comments, Grant.

welcome :)
kudos for chasing this.

...
> >> +		/* store the drive's geometry info we just read */
> >> +		sf_heads = page5->number_of_heads;
> >> +		sf_sectors = page5->sectors_per_track;
> >> +		sf_cylinders = page5->number_of_cylinders_msb << 8
> >> +		               | page5->number_of_cylinders_lsb;
> > 
> > I'm kinda nervous about the use of static globals to "return" the
> > result of the previous code. In case some whacko decided to
> > have two scsi floppies with different media types, would this break?
> 
> I also agree.  In retrospect, I shouldn't have done this the way I did.  I
> mentioned this to Helge earlier, and I've already changed these to static
> consts in my source, since I'm basically hard-coding the FC-1's geometry
> and consts allow me to sanity check the mode select that sets the geometry.
> Does that sound logical?

It sounds good but it's totally not obvious this was the intent.
If you are "hard coding" the geometry, then can't you just use #define?

I think it's ok to only support one geometry.
Just add a check, something like this:
	if (sf_heads) {
		/* make sure successive floppies have same geometry */
		if (sf_heads != page5->number_of_heads) {
			printk(KERN_ERR "Sorry, only support one geometry"
					" for SCSI floppy.\n");
			return -ENOMEDIA;
		} 
	} else {
		sf_heads =....
	}

And maybe think about a better error message and how to handle it. YKWIM.

>   Technically, it wouldn't have broken the way it is written
> because the geometry should always be the same for these drives,
> but it wasn't the best implementation.

ok. That makes sense. but "these drives" are a specific model of drive
with a specific type of media installed.  I don't see checks for that here.
I expect there are other "device->removable && !TYPE_MOD" devices.

> I'll wait a bit longer to see if I get any more comments, then I'll post an
> updated patch that will include your suggestions.

Sounds good - many thanks!

grant
_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux

Re: [parisc-linux] [PATCH] sd: Adds flexible disk (TEAC FC-1, scsi-floppy) support to scsi disk driver

[James K. Love]

On 3/8/2007 1:26 PM, Grant Grundler wrote:
> welcome :)
> kudos for chasing this.

No Problem.  I can't think of anything more exciting than finally getting an
obscure, antiquated, low-capacity, franken-drive like the TEAC FC-1 working.
But seriously, I hope someone else finds this patch useful.

> It sounds good but it's totally not obvious this was the intent.
> If you are "hard coding" the geometry, then can't you just use #define?

Yes or consts, whichever is preferable.  The drive geometry is always initially
incorrect, so I must set it to the correct values.  These values should always
be the same for THIS drive... which leads me to...

> I expect there are other "device->removable && !TYPE_MOD" devices.

You would know this better than I, so I'll defer to you.  I was always a bit
uncomfortable making this assumption, which I documented in a comment in the
code.  When the scsi driver attaches to my devices, they are all of TYPE_DISK
and removable.  I was hoping someone else could test this out with a different
scsi-floppy drive, but there may not be any others.  Would it be a better idea
to use model and vendor information to exactly identify the drive as a TEAC FC-1?

Regards,
James Love

_______________________________________________
parisc-linux mailing list
parisc-linux@lists.parisc-linux.org
http://lists.parisc-linux.org/mailman/listinfo/parisc-linux