Re: [PATCH 23/31] swim: Don't needlessly re-read sectors

Le 16/07/2026 à 12:02, Finn Thain a écrit :
> floppy_read_sectors() is confusing because variable names seem to conflate
> tracks and cylinders. Rename the track variable, eliminate a division
> operation and adopt suitable integer types.
> 
> For readahead to work effectively, small sequential reads should not
> require waiting for spindle rotation. Unfortunately, the present algorithm
> is very inefficient and does a lot of unnecessary waiting.
> 
> E.g. if the device is asked to read sectors 1 thru 18, and if sector 9
> happens to be under the heads, the driver will proceed to read sectors 9
> thru 18, but discard the results, while it waits for sector 1 to arrive.
> 
> If sector 1 couldn't be read on the first attempt, and needs a retry, the
> driver will proceed to read sectors 2 thru 18, but discard the results,
> while it waits for sector 1 to come around again.
> 
> In between reading sector 1 and sector 2, the driver needlessly calls
> swim_track() and swim_head() again. But what's worse is that, on a 68030
> system, re-enabling interrupts after each sector read can result in a full
> rotation between sectors (that is a 3 ms wait).
> 
> Floppy drivers often implement a track cache, that can be filled in a
> single rotation, to solve problems like these. But I think this solution
> is much simpler.
> 
> For each request, use a sector bitmap to keep a record of sectors read
> successfully. Read or retry, as necessary, the requested sectors in
> whatever sequence they arrive in. Keep interrupts disabled until the
> whole track has passed under the read head.
> 
> Fixes: 8852ecd97488 ("m68k: mac - Add SWIM floppy support")
> Signed-off-by: Finn Thain <[email protected]>
> ---
>   drivers/block/swim.c | 91 +++++++++++++++++++++++++-------------------
>   1 file changed, 52 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/block/swim.c b/drivers/block/swim.c
> index 0bab7ed3daa8..dae401e649d7 100644
> --- a/drivers/block/swim.c
> +++ b/drivers/block/swim.c
> @@ -180,9 +180,9 @@ struct floppy_state {
>   	enum media_type	 type;
>   	int		 write_protected;
>   
> -	int		 total_secs;
> -	int		 secpercyl;
> -	int		 secpertrack;
> +	unsigned int	 total_secs;
> +	unsigned int	 secpercyl;
> +	unsigned int	 secpertrack;
>   
>   	/* in-use information */
>   
> @@ -455,68 +455,81 @@ static int floppy_eject(struct floppy_state *fs)
>   	return 0;
>   }
>   
> -static inline int swim_read_sector(struct floppy_state *fs,
> -				   int side, int track,
> -				   int sector, unsigned char *buffer)
> +static unsigned int swim_read_sector_range(struct floppy_state *fs,
> +					unsigned int side, unsigned int track,
> +					unsigned int start, unsigned int count,
> +					unsigned char *buffer)
>   {
>   	struct swim __iomem *base = fs->swd->base;
>   	unsigned long flags;
>   	struct sector_header header;
> -	int ret = -1;
> -	short i;
> +	int i, bits = 0;
>   
> -	swim_track(fs, track);
> -	swim_head(base, side);
> +	for (i = 0; i < 18; i++) { /* A track has at most 18 sectors */
> +		if (i >= count)
> +			break;
> +		bits |= BIT(i);
> +	}
>   
>   	local_irq_save(flags);
>   	for (i = 0; i < 36; i++) {
> +		if (bits == 0) /* All count sectors were read ok */
> +			break;
> +
>   		if (swim_read_sector_header(base, &header) ||
>   		    swim_read(base, error) || header.track != track ||
>   		    header.side != side || header.size != 2)
>   			continue;
> -		if (header.sector == sector) {
> -			/* found */
>   
> -			ret = swim_read_sector_data(base, buffer);
> -			if (swim_read(base, error))
> -				ret = -EIO;
> -			break;
> +		if (header.sector >= start && header.sector < start + count) {
> +			unsigned int offset = header.sector - start;
> +			int ret;
> +
> +			if ((bits & BIT(offset)) == 0)
> +				continue; /* This sector was already read ok */
> +
> +			ret = swim_read_sector_data(base, buffer + 512 * offset);
> +			if (ret == 512 && swim_read(base, error) == 0)
> +				bits &= ~BIT(offset);
>   		}
>   	}
>   	local_irq_restore(flags);
>   
> -	return ret;
> +	return bits ? ffs(bits) - 1 : count; /* No. of contiguous ok sectors */
>   }
>   
>   static blk_status_t floppy_read_sectors(struct floppy_state *fs,
> -			       int req_sector, int sectors_nb,
> -			       unsigned char *buffer)
> +					unsigned int req_sector,
> +					unsigned int sectors_nb,
> +					unsigned char *buffer)
>   {
>   	struct swim __iomem *base = fs->swd->base;
> -	int ret;
> -	int side, track, sector;
> -	int i, try;
> -
> +	unsigned int failures = 0;
>   
>   	swim_drive(base, fs->location);
>   	swim_READY_timeout(base);
>   
> -	for (i = req_sector; i < req_sector + sectors_nb; i++) {
> -		int x;
> -		track = i / fs->secpercyl;
> -		x = i % fs->secpercyl;
> -		side = x / fs->secpertrack;
> -		sector = x % fs->secpertrack + 1;
> -
> -		try = 5;
> -		do {
> -			ret = swim_read_sector(fs, side, track, sector,
> -						buffer);
> -			if (try-- == 0)
> -				return BLK_STS_IOERR;
> -		} while (ret != 512);
> -
> -		buffer += ret;
> +	while (sectors_nb) {
> +		unsigned int cyl, x, head, sector, n, ret;
> +
> +		cyl = req_sector / fs->secpercyl;
> +		x = req_sector % fs->secpercyl;
> +		head = (x >= fs->secpertrack) ? 1 : 0;
> +		sector = x % fs->secpertrack;
> +		n = min(sectors_nb, fs->secpertrack - sector);
> +
> +		swim_track(fs, cyl);
> +		swim_head(base, head);
> +
> +		ret = swim_read_sector_range(fs, head, cyl, sector + 1, n, buffer);
> +		if (ret != n)
> +			++failures;
> +		if (failures >= 5)
> +			return BLK_STS_IOERR;

Old code failed after 5 retries on the same sector, new code fails after 5 incomplete ranges 
anywhere in it, even if earlier errors were recovered and request made progress.

Is this what you want?

Thanks,
Laurent

> +
> +		buffer += 512 * ret;
> +		sectors_nb -= ret;
> +		req_sector += ret;
>   	}
>   
>   	return 0;