svn commit: r200481 - head/sys/dev/ata

[Marius Strobl]

On Sun, Dec 13, 2009 at 11:44:13AM -0800, Sam Leffler wrote:
> Marius Strobl wrote:
> >Author: marius
> >Date: Sun Dec 13 18:26:19 2009
> >New Revision: 200481
> >URL: http://svn.freebsd.org/changeset/base/200481
> >
> >Log:
> >  Specify the capability and media bits of the capabilities page in
> >  native, i.e. big-endian, format and convert as appropriate like we
> >  also do with the multibyte fields of the other pages. This fixes
> >  the output of acd_describe() to match reality on big-endian machines
> >  without breaking it on little-endian ones. While at it, also convert
> >  the remaining multibyte fields of the pages read although they are
> >  currently unused for consistency and in order to prevent possible
> >  similar bugs in the future.
> >  
> >  MFC after:	1 week
> >
> >Modified:
> >  head/sys/dev/ata/atapi-cd.c
> >  head/sys/dev/ata/atapi-cd.h
> >
> >Modified: head/sys/dev/ata/atapi-cd.c
> >==============================================================================
> >--- head/sys/dev/ata/atapi-cd.c	Sun Dec 13 17:49:22 2009 (r200480)
> >+++ head/sys/dev/ata/atapi-cd.c	Sun Dec 13 18:26:19 2009 (r200481)
> >@@ -1206,6 +1206,7 @@ acd_read_track_info(device_t dev, int32_
> >     if ((error = ata_atapicmd(dev, ccb, (caddr_t)info, sizeof(*info),
> > 			      ATA_R_READ, 30)))
> > 	return error;
> >+    info->data_length = ntohs(info->data_length);
> >     info->track_start_addr = ntohl(info->track_start_addr);
> >     info->next_writeable_addr = ntohl(info->next_writeable_addr);
> >     info->free_blocks = ntohl(info->free_blocks);
> >@@ -1644,12 +1645,17 @@ acd_get_cap(device_t dev)
> >     for (count = 0 ; count < 5 ; count++) {
> > 	if (!ata_atapicmd(dev, ccb, (caddr_t)&cdp->cap, sizeof(cdp->cap),
> > 			  ATA_R_READ | ATA_R_QUIET, 5)) {
> >+	    cdp->cap.data_length = ntohs(cdp->cap.data_length);
> >+	    cdp->cap.blk_desc_len = ntohs(cdp->cap.blk_desc_len);
> >+	    cdp->cap.media = ntohs(cdp->cap.media);
> >+	    cdp->cap.capabilities = ntohs(cdp->cap.capabilities);
> > 	    cdp->cap.max_read_speed = ntohs(cdp->cap.max_read_speed);
> >+	    cdp->cap.max_vol_levels = ntohs(cdp->cap.max_vol_levels);
> >+	    cdp->cap.buf_size = ntohs(cdp->cap.buf_size);
> > 	    cdp->cap.cur_read_speed = ntohs(cdp->cap.cur_read_speed);
> > 	    cdp->cap.max_write_speed = ntohs(cdp->cap.max_write_speed);
> > 	    cdp->cap.cur_write_speed = 
> > 	    max(ntohs(cdp->cap.cur_write_speed),177);
> >-	    cdp->cap.max_vol_levels = ntohs(cdp->cap.max_vol_levels);
> >-	    cdp->cap.buf_size = ntohs(cdp->cap.buf_size);
> >+	    cdp->cap.copy_protect_rev = ntohs(cdp->cap.copy_protect_rev);
> > 	}
> >     }
> > }
> 
> I don't think this should use ntoh* but instead use le*/be* macros.
> 

I agree but ata(4) uses hton*(9) and ntoh*(9) throughout its source
so changing it to use bytorder(9) functions instead should be a
separate style change.

> Separately the ata code was very broken in it's handling of big-endian 
> byte order; e.g. for arm I had to supply wrong bus space methods as a 
> workaround.  Do these changes take this byte-order confusion into account?
> 

At least on sparc64 I haven't seen such problems. There all ATA
controllers are PCI which uses little-endian bus space accessors.
AFAICT at least atapi-cd(4) now does the right things as the
native byteorder of the pages indeed is big-endian as explicitly
mentioned in MMC-5.

Marius