Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio

[Li Zhong <zhong@linux.vnet.ibm.com>]

On Wed, 2013-11-20 at 12:28 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2013-11-19 at 16:11 +0800, Li Zhong wrote:
> > I encountered following issue:
> > [    0.283035] ibmvscsi 30000015: couldn't initialize event pool
> > [    5.688822] ibmvscsi: probe of 30000015 failed with error -1
> > 
> > which prevents the storage from being recognized, and the machine from
> > booting.
> > 
> > After some digging, it seems that it is caused by commit 4886c399da
> > 
> > as dma_mask pointer in viodev->dev is not set, so in
> > dma_set_mask_and_coherent(), dma_set_coherent_mask() is not called
> > because dma_set_mask(), which is dma_set_mask_pSeriesLP() returned EIO.
> > While before the commit, dma_set_coherent_mask() is always called. 
> > 
> > I tried to replace dma_set_mask_and_coherent() with
> > dma_coerce_mask_and_coherent(), and the machine could boot again. 
> > 
> > But I'm not sure whether this is the correct fix...
> 
> Russell, care to chime in ? I can't make sense of the semantics...
> 
> The original commit was fairly clear:
> 
> <<
>     Replace the following sequence:
>     
>     	dma_set_mask(dev, mask);
>     	dma_set_coherent_mask(dev, mask);
>     
>     with a call to the new helper dma_set_mask_and_coherent().
> >>
> 
> It all makes sense so far ... but doesn't work for some odd reason,
> and the "fix" uses a function whose name doesn't make much sense to
> me ... what is the difference between "setting" and "coercing"
> the mask ? And why doe replacing two "set" with a "set both" doesn't
> work and require a coerce ?

I think the difference is because the check of return value from
dma_set_mask in dma_coerce_mask_and_coherent():
--
        int rc = dma_set_mask(dev, mask);
        if (rc == 0)
                dma_set_coherent_mask(dev, mask);
--

and in struct device {, dma_mask is a pointer, while coherent_dma_mask
is value (don't know why we have this difference). 

And here for pseries, dma_set_mask() failed because the dma_mask pointer
still remains null.

And in dma_coerce_mask_and_coherent(), the dma_mask is set with the
address of coherent_dma_mask
--
	dev->dma_mask = &dev->coherent_dma_mask;
--

Thanks, Zhong

> 
> I'm asking because I'm worried about breakage elsewhere...
> 
> Cheers,
> Ben. 
> 
> > ---
> >  arch/powerpc/kernel/vio.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
> > index e7d0c88..76a6482 100644
> > --- a/arch/powerpc/kernel/vio.c
> > +++ b/arch/powerpc/kernel/vio.c
> > @@ -1419,7 +1419,7 @@ struct vio_dev *vio_register_device_node(struct device_node *of_node)
> >  
> >  		/* needed to ensure proper operation of coherent allocations
> >  		 * later, in case driver doesn't set it explicitly */
> > -		dma_set_mask_and_coherent(&viodev->dev, DMA_BIT_MASK(64));
> > +		dma_coerce_mask_and_coherent(&viodev->dev, DMA_BIT_MASK(64));
> >  	}
> >  
> >  	/* register with generic device framework */
> > 
> > 
> > 
> > 
> 
>