svn commit: r357647 - in head/sys: cam/ata cam/mmc cam/nvme cam/scsi dev/aac dev/altera/avgen dev/altera/sdcard dev/amr dev/cfi dev/flash dev/ida dev/ips dev/mfi dev/mlx dev/mmc dev/nvme dev/pst de...

[Dimitry Andric]

On 7 Feb 2020, at 10:22, Scott Long <scottl at freebsd.org> wrote:
> 
> Author: scottl
> Date: Fri Feb  7 09:22:08 2020
> New Revision: 357647
> URL: https://svnweb.freebsd.org/changeset/base/357647
> 
> Log:
>  Ever since the block layer expanded its command syntax beyond just
>  BIO_READ and BIO_WRITE, we've handled this expanded syntax poorly in
>  drivers when the driver doesn't support a particular command.  Do a
>  sweep and fix that.
...
> Modified: head/sys/dev/altera/sdcard/altera_sdcard_io.c
> ==============================================================================
> --- head/sys/dev/altera/sdcard/altera_sdcard_io.c	Fri Feb  7 08:39:00 2020	(r357646)
> +++ head/sys/dev/altera/sdcard/altera_sdcard_io.c	Fri Feb  7 09:22:08 2020	(r357647)
> @@ -293,27 +293,27 @@ recheck:
> }
> 
> static void
> -altera_sdcard_io_start_internal(struct altera_sdcard_softc *sc, struct bio *bp)
> +altera_sdcard_io_start_internal(struct altera_sdcard_softc *sc, struct bio **bp)
> {
> 
> -	switch (bp->bio_cmd) {
> +	switch (*bp->bio_cmd) {
> 	case BIO_READ:
> -		altera_sdcard_write_cmd_arg(sc, bp->bio_pblkno *
> +		altera_sdcard_write_cmd_arg(sc, *bp->bio_pblkno *
> 		    ALTERA_SDCARD_SECTORSIZE);
> 		altera_sdcard_write_cmd(sc, ALTERA_SDCARD_CMD_READ_BLOCK);
> 		break;
> 
> 	case BIO_WRITE:
> -		altera_sdcard_write_rxtx_buffer(sc, bp->bio_data,
> -		    bp->bio_bcount);
> -		altera_sdcard_write_cmd_arg(sc, bp->bio_pblkno *
> +		altera_sdcard_write_rxtx_buffer(sc, *bp->bio_data,
> +		    *bp->bio_bcount);
> +		altera_sdcard_write_cmd_arg(sc, *bp->bio_pblkno *
> 		    ALTERA_SDCARD_SECTORSIZE);
> 		altera_sdcard_write_cmd(sc, ALTERA_SDCARD_CMD_WRITE_BLOCK);
> 		break;
> 
> 	default:
> -		panic("%s: unsupported I/O operation %d", __func__,
> -		    bp->bio_cmd);
> +		biofinish(*bp, NULL, EOPNOTSUPP);
> +		*bp = NULL;
> 	}
> }
> 

This gets the indirections wrong, leading to errors with at least clang 10:

sys/dev/altera/sdcard/altera_sdcard_io.c: In function 'altera_sdcard_io_start_internal':
sys/dev/altera/sdcard/altera_sdcard_io.c:299:13: error: '*bp' is a pointer; did you mean to use '->'?
  switch (*bp->bio_cmd) {
             ^~
             ->
sys/dev/altera/sdcard/altera_sdcard_io.c:301:38: error: '*bp' is a pointer; did you mean to use '->'?
   altera_sdcard_write_cmd_arg(sc, *bp->bio_pblkno *
                                      ^~
                                      ->
sys/dev/altera/sdcard/altera_sdcard_io.c:307:42: error: '*bp' is a pointer; did you mean to use '->'?
   altera_sdcard_write_rxtx_buffer(sc, *bp->bio_data,
                                          ^~
                                          ->
sys/dev/altera/sdcard/altera_sdcard_io.c:308:10: error: '*bp' is a pointer; did you mean to use '->'?
       *bp->bio_bcount);
          ^~
          ->
sys/dev/altera/sdcard/altera_sdcard_io.c:309:38: error: '*bp' is a pointer; did you mean to use '->'?
   altera_sdcard_write_cmd_arg(sc, *bp->bio_pblkno *
                                      ^~
                                      ->

In this case, (*bp)->fieldname is the correct way to specify the struct fields.


> @@ -332,8 +332,8 @@ altera_sdcard_io_start(struct altera_sdcard_softc *sc,
> 	 */
> 	KASSERT(bp->bio_bcount == ALTERA_SDCARD_SECTORSIZE,
> 	    ("%s: I/O size not %d", __func__, ALTERA_SDCARD_SECTORSIZE));
> -	altera_sdcard_io_start_internal(sc, bp);
> -	sc->as_currentbio = bp;
> +	altera_sdcard_io_start_internal(sc, &bp);
> +	sc->as_currentbio = *bp;
> 	sc->as_retriesleft = ALTERA_SDCARD_RETRY_LIMIT;
> }

And a similar error here:

sys/dev/altera/sdcard/altera_sdcard_io.c: In function 'altera_sdcard_io_start':
sys/dev/altera/sdcard/altera_sdcard_io.c:336:20: error: incompatible types when assigning to type 'struct bio *' from type 'struct bio'
  sc->as_currentbio = *bp;
                    ^

As sc->as_currentbio is already a pointer, there is no need to dereference bp again.

I submitted https://reviews.freebsd.org/D23730 to get this fixed.

-Dimitry

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 223 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20200217/3fdfbdea/attachment.sig>