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

Le 16/07/2026 =C3=A0 12:02, Finn Thain a =C3=A9crit=C2=A0:
> floppy_read_sectors() is confusing because variable names seem to confla=
te
> tracks and cylinders. Rename the track variable, eliminate a division
> operation and adopt suitable integer types.
>=20
> For readahead to work effectively, small sequential reads should not
> require waiting for spindle rotation. Unfortunately, the present algorit=
hm
> is very inefficient and does a lot of unnecessary waiting.
>=20
> 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.
>=20
> If sector 1 couldn't be read on the first attempt, and needs a retry, th=
e
> driver will proceed to read sectors 2 thru 18, but discard the results,
> while it waits for sector 1 to come around again.
>=20
> 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 fu=
ll
> rotation between sectors (that is a 3 ms wait).
>=20
> 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.
>=20
> 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.
>=20
> 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(-)
>=20
> 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;
>  =20
> -	int		 total_secs;
> -	int		 secpercyl;
> -	int		 secpertrack;
> +	unsigned int	 total_secs;
> +	unsigned int	 secpercyl;
> +	unsigned int	 secpertrack;
>  =20
>   	/* in-use information */
>  =20
> @@ -455,68 +455,81 @@ static int floppy_eject(struct floppy_state *fs)
>   	return 0;
>   }
>  =20
> -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 =3D fs->swd->base;
>   	unsigned long flags;
>   	struct sector_header header;
> -	int ret =3D -1;
> -	short i;
> +	int i, bits =3D 0;
>  =20
> -	swim_track(fs, track);
> -	swim_head(base, side);
> +	for (i =3D 0; i < 18; i++) { /* A track has at most 18 sectors */
> +		if (i >=3D count)
> +			break;
> +		bits |=3D BIT(i);
> +	}
>  =20
>   	local_irq_save(flags);
>   	for (i =3D 0; i < 36; i++) {
> +		if (bits =3D=3D 0) /* All count sectors were read ok */
> +			break;
> +
>   		if (swim_read_sector_header(base, &header) ||
>   		    swim_read(base, error) || header.track !=3D track ||
>   		    header.side !=3D side || header.size !=3D 2)
>   			continue;
> -		if (header.sector =3D=3D sector) {
> -			/* found */
>  =20
> -			ret =3D swim_read_sector_data(base, buffer);
> -			if (swim_read(base, error))
> -				ret =3D -EIO;
> -			break;
> +		if (header.sector >=3D start && header.sector < start + count) {
> +			unsigned int offset =3D header.sector - start;
> +			int ret;
> +
> +			if ((bits & BIT(offset)) =3D=3D 0)
> +				continue; /* This sector was already read ok */
> +
> +			ret =3D swim_read_sector_data(base, buffer + 512 * offset);
> +			if (ret =3D=3D 512 && swim_read(base, error) =3D=3D 0)
> +				bits &=3D ~BIT(offset);
>   		}
>   	}
>   	local_irq_restore(flags);
>  =20
> -	return ret;
> +	return bits ? ffs(bits) - 1 : count; /* No. of contiguous ok sectors *=
/
>   }
>  =20
>   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 =3D fs->swd->base;
> -	int ret;
> -	int side, track, sector;
> -	int i, try;
> -
> +	unsigned int failures =3D 0;
>  =20
>   	swim_drive(base, fs->location);
>   	swim_READY_timeout(base);
>  =20
> -	for (i =3D req_sector; i < req_sector + sectors_nb; i++) {
> -		int x;
> -		track =3D i / fs->secpercyl;
> -		x =3D i % fs->secpercyl;
> -		side =3D x / fs->secpertrack;
> -		sector =3D x % fs->secpertrack + 1;
> -
> -		try =3D 5;
> -		do {
> -			ret =3D swim_read_sector(fs, side, track, sector,
> -						buffer);
> -			if (try-- =3D=3D 0)
> -				return BLK_STS_IOERR;
> -		} while (ret !=3D 512);
> -
> -		buffer +=3D ret;
> +	while (sectors_nb) {
> +		unsigned int cyl, x, head, sector, n, ret;
> +
> +		cyl =3D req_sector / fs->secpercyl;
> +		x =3D req_sector % fs->secpercyl;
> +		head =3D (x >=3D fs->secpertrack) ? 1 : 0;
> +		sector =3D x % fs->secpertrack;
> +		n =3D min(sectors_nb, fs->secpertrack - sector);
> +
> +		swim_track(fs, cyl);
> +		swim_head(base, head);
> +
> +		ret =3D swim_read_sector_range(fs, head, cyl, sector + 1, n, buffer);
> +		if (ret !=3D n)
> +			++failures;
> +		if (failures >=3D 5)
> +			return BLK_STS_IOERR;

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

Is this what you want?

Thanks,
Laurent

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