Re: [PATCH] floppy: support arbitrary first-sector numbers

[Andrew Morton]

On Sat, 9 Aug 2008 19:42:14 +0100
Keith Wansbrough <keith@xxxxxxxxxx> wrote:

> The current floppy_struct allows floppies to number sectors starting
> from 0 or 1.  This patch allows arbitrary first-sector numbers - for
> example, 0xC1 for Amstrad CPC disks.
> 
> This extends the existing 1-bit field (FD_ZEROBASED, bit 2 of stretch) 
> to 8 bits (FD_SECTMASK, bits 2 to 9).
> 
> Currently 0x00 denotes a first sector number of 1, and 0x01 denotes a
> first sector number of 0.  We extend this by interpreting FD_SECTMASK
> as the first sector number with the LSB flipped.
> 
> Signed-off-by: Keith Wansbrough <keith@xxxxxxxxxx>
> ---
> 
> I've tested formatting, writing, and reading disks with a variety of
> first sector numbers (including 0 and 1), and all works as expected.
> 
> I have made corresponding changes to fdutils, which I will submit
> separately to the fdutils maintainer.
> 
> There's no floppy maintainer listed in MAINTAINERS, so this is sent to
> the block subsystem maintainer and to the fdutils maintainer.  I'm cc:ing
> Gene because I know he's been working in this area recently.
> 
> --KW 8-)
> 
> Index: linux-2.6.27-rc1-git4/drivers/block/floppy.c
> ===================================================================
> --- linux-2.6.27-rc1-git4.orig/drivers/block/floppy.c	2008-08-02 23:36:32.000000000 +0100
> +++ linux-2.6.27-rc1-git4/drivers/block/floppy.c	2008-08-02 23:36:37.000000000 +0100
> @@ -423,8 +423,15 @@ static struct floppy_raw_cmd *raw_cmd, d
>   * 1581's logical side 0 is on physical side 1, whereas the Sharp's logical
>   * side 0 is on physical side 0 (but with the misnamed sector IDs).
>   * 'stretch' should probably be renamed to something more general, like
> - * 'options'.  Other parameters should be self-explanatory (see also
> - * setfdprm(8)).
> + * 'options'.
> + *
> + * Bits 2 through 9 of 'stretch' tell the number of the first sector.
> + * The LSB (bit 2) is flipped. For most disks, the first sector
> + * is 1 (represented by 0x00<<2).  For some CP/M and music sampler
> + * disks (such as Ensoniq EPS 16plus) it is 0 (represented as 0x01<<2).
> + * For Amstrad CPC disks it is 0xC1 (represented as 0xC0<<2).
> + *
> + * Other parameters should be self-explanatory (see also setfdprm(8)).
>   */
>  /*
>  	    Size
> @@ -2236,9 +2243,9 @@ static void setup_format_params(int trac
>  			}
>  		}
>  	}
> -	if (_floppy->stretch & FD_ZEROBASED) {
> +	if (_floppy->stretch & FD_SECTBASEMASK) {
>  		for (count = 0; count < F_SECT_PER_TRACK; count++)
> -			here[count].sect--;
> +			here[count].sect += FD_SECTBASE(_floppy) - 1;
>  	}
>  }
>  
> @@ -2649,7 +2656,7 @@ static int make_raw_rw_request(void)
>  	}
>  	HEAD = fsector_t / _floppy->sect;
>  
> -	if (((_floppy->stretch & (FD_SWAPSIDES | FD_ZEROBASED)) ||
> +	if (((_floppy->stretch & (FD_SWAPSIDES | FD_SECTBASEMASK)) ||
>  	     TESTF(FD_NEED_TWADDLE)) && fsector_t < _floppy->sect)
>  		max_sector = _floppy->sect;
>  
> @@ -2679,7 +2686,7 @@ static int make_raw_rw_request(void)
>  	CODE2SIZE;
>  	SECT_PER_TRACK = _floppy->sect << 2 >> SIZECODE;
>  	SECTOR = ((fsector_t % _floppy->sect) << 2 >> SIZECODE) +
> -	    ((_floppy->stretch & FD_ZEROBASED) ? 0 : 1);
> +	    FD_SECTBASE(_floppy);
>  
>  	/* tracksize describes the size which can be filled up with sectors
>  	 * of size ssize.
> @@ -3311,7 +3318,7 @@ static inline int set_geometry(unsigned 
>  	    g->head <= 0 ||
>  	    g->track <= 0 || g->track > UDP->tracks >> STRETCH(g) ||
>  	    /* check if reserved bits are set */
> -	    (g->stretch & ~(FD_STRETCH | FD_SWAPSIDES | FD_ZEROBASED)) != 0)
> +	    (g->stretch & ~(FD_STRETCH | FD_SWAPSIDES | FD_SECTBASEMASK)) != 0)
>  		return -EINVAL;
>  	if (type) {
>  		if (!capable(CAP_SYS_ADMIN))
> @@ -3356,7 +3363,7 @@ static inline int set_geometry(unsigned 
>  		if (DRS->maxblock > user_params[drive].sect ||
>  		    DRS->maxtrack ||
>  		    ((user_params[drive].sect ^ oldStretch) &
> -		     (FD_SWAPSIDES | FD_ZEROBASED)))
> +		     (FD_SWAPSIDES | FD_SECTBASEMASK)))
>  			invalidate_drive(bdev);
>  		else
>  			process_fd_request();
> Index: linux-2.6.27-rc1-git4/include/linux/fd.h
> ===================================================================
> --- linux-2.6.27-rc1-git4.orig/include/linux/fd.h	2008-08-02 23:36:32.000000000 +0100
> +++ linux-2.6.27-rc1-git4/include/linux/fd.h	2008-08-02 23:36:37.000000000 +0100
> @@ -15,10 +15,16 @@ struct floppy_struct {
>  			sect,		/* sectors per track */
>  			head,		/* nr of heads */
>  			track,		/* nr of tracks */
> -			stretch;	/* !=0 means double track steps */
> +			stretch;	/* bit 0 !=0 means double track steps */
> +					/* bit 1 != 0 means swap sides */
> +					/* bits 2..9 give the first sector */
> +					/*  number (the LSB is flipped) */
>  #define FD_STRETCH 1
>  #define FD_SWAPSIDES 2
>  #define FD_ZEROBASED 4
> +#define FD_SECTBASEMASK 0x3FC
> +#define FD_MKSECTBASE(s) (((s) ^ 1) << 2)
> +#define FD_SECTBASE(floppy) ((((floppy)->stretch & FD_SECTBASEMASK) >> 2) ^ 1)
>  
>  	unsigned char	gap,		/* gap1 size */
>  

So this is a backward-compatible enhancement to the floppy FDSETPRM and
FDDEFPRM ioctl.  If some userspace wasn't setting those bits to zero,
it loses, and probably deserved to.

I'd suggest an update to the FDDEFPRM documentation, only it isn't
documented.

Who is the fdutils maintainer?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/