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

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