[illumos-Developer] vmxnet3s driver failing on debug builds due to dma_attr_sgllen == -1 (Bug #422)

[Garrett D'Amore]

On Sun, 2011-02-06 at 11:11 -0600, Mike La Spina wrote:
> The original call is validating the maxsegmentsize which is not a direct
> function of the sgllen parm. The issue becomes does removing the OR
> sgllen <= 0 invalidate the entire test? I think it does not, but there
> may be a reason to do a sanity check on sgllen which seems that the test
> should actually return BAD_ATTR except when sgllen == -1. 

Removing it doesn't invalidate the entire test, but we should be
checking against the sgllen == 0.   Other negative values are explicitly
allowed in the doco (though I can't imagine why it would ever be
anything except -1.)

	- Garrett
> 
> -----Original Message-----
> From: Yuri Pankov [mailto:yuri.pankov at gmail.com] 
> Sent: Sunday, February 06, 2011 5:11 AM
> To: Garrett D'Amore
> Cc: Mike La Spina; developer at lists.illumos.org
> Subject: Re: [illumos-Developer] vmxnet3s driver failing on debug builds
> due to dma_attr_sgllen == -1 (Bug #422)
> 
> On Sat, Feb 05, 2011 at 09:35:55PM -0800, Garrett D'Amore wrote:
> > I've finally looked at this.  I agree, its a bug for DEBUG kernels.
> > Lets file a bug in illumos, and we'll fix it.
> > 
> > 	- Garrett
> > 
> > On Sat, 2011-02-05 at 23:19 -0600, Mike La Spina wrote:
> > > I would think that the debug call code is not appropriate. There is
> > > nothing wrong with the -1 value and it should not return a bad
> attribute
> > > when it's valid.
> > > 
> 
> That's cool and all. Could you comment on the issue please? Why the
> check was there? Aren't we doing something bad removing the check (may
> be we should make it more clearer)?
> 
> > > -----Original Message-----
> > > From: Yuri Pankov [mailto:yuri.pankov at gmail.com] 
> > > Sent: Saturday, February 05, 2011 10:54 AM
> > > To: Garrett D'Amore
> > > Cc: developer at lists.illumos.org
> > > Subject: Re: [illumos-Developer] vmxnet3s driver failing on debug
> builds
> > > due to dma_attr_sgllen == -1 (Bug #422)
> > > 
> > > On Sat, Feb 05, 2011 at 07:34:56AM -0800, Garrett D'Amore wrote:
> > > > On Sat, 2011-02-05 at 10:29 +0300, Yuri Pankov wrote:
> > > > > Hi,
> > > > > 
> > > > > While looking into the issue, I've found the following piece of
> code
> > > in
> > > > > usr/src/uts/i86pc/io/rootnex.c, called only in debug builds:
> > > > > 
> > > > > /*
> > > > >  * rootnex_valid_alloc_parms()
> > > > >  *    Called in ddi_dma_alloc_handle path to validate its
> > > parameters.
> > > > >  */
> > > > > static int
> > > > > rootnex_valid_alloc_parms(ddi_dma_attr_t *attr, uint_t
> > > maxsegmentsize)
> > > > > ...
> > > > >         if ((attr->dma_attr_seg & MMU_PAGEOFFSET) !=
> MMU_PAGEOFFSET
> > > ||
> > > > >             MMU_PAGESIZE & (attr->dma_attr_granular - 1) ||
> > > > > ==>         attr->dma_attr_sgllen <= 0) {          <===
> > > > >                 return (DDI_DMA_BADATTR);
> > > > >         }
> > > > > ...
> > > > > 
> > > > > vmxnet3s driver uses dma_attr_sgllen == -1, which is pretty ok
> > > according
> > > > > to ddi_dma_attr(9S):
> > > > > ...
> > > > >      < 0    Device DMA engine is not constrained by the size,
> for
> > > > >             example, withDMA chaining.
> > > > > ...
> > > > > 
> > > > > Removing the check allows the driver to successfully start (and
> > > > > function, I haven't tested the 10Gbps speed due to the lack of
> > > hardware,
> > > > > but currently finishing 50GB transfer at 1Gbps speed). The check
> is
> > > > > there from the OpenSolaris launch, so the my real question is -
> what
> > > of
> > > > > the following sounds correct:
> > > > > - the check is bogus and should be removed (or changed to == 0,
> > > which is
> > > > >   reserved per manpage)
> > > > > or
> > > > > - documentation (ddi_dma_attr(9S), at least) should be updated
> and
> > > the
> > > > >   driver should be "fixed".
> > > > > 
> > > > 
> > > > I haven't looked yet, but lots of drivers have sgllen == -1.  That
> > > means
> > > > your driver can accept arbitrary chaining via linked lists.  I
> expect
> > > > there is something else going on here, and I'll look at more
> closely
> > > as
> > > > soon as I have time.
> > > > 
> > > > 	- Garrett
> > > 
> > > Thanks Garrett.
> > > 
> > > I just wasn't able to find any driver in tree using sgllen = -1
> (that
> > > was just some grepping though) and I don't really see how allocation
> > > could succeed given the codepath. Could you please provide some
> examples
> > > of drivers using sgllen = -1? Sorry if I'm missing something obvious
> > > here, have much to learn yet..
> > > 
> > > 
> > > TIA,
> > > Yuri
> > > 
> > > _______________________________________________
> > > Developer mailing list
> > > Developer at lists.illumos.org
> > > http://lists.illumos.org/m/listinfo/developer
> > 
> >