* [RFC PATCH powerpc] Fix a dma_mask issue of vio
@ 2013-11-19 8:11 Li Zhong
2013-11-20 1:28 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 9+ messages in thread
From: Li Zhong @ 2013-11-19 8:11 UTC (permalink / raw)
To: PowerPC email list; +Cc: Paul Mackerras, rmk+kernel
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...
---
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 */
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-19 8:11 [RFC PATCH powerpc] Fix a dma_mask issue of vio Li Zhong
@ 2013-11-20 1:28 ` Benjamin Herrenschmidt
2013-11-20 2:04 ` Li Zhong
2013-11-20 23:23 ` Russell King - ARM Linux
0 siblings, 2 replies; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2013-11-20 1:28 UTC (permalink / raw)
To: Li Zhong; +Cc: Paul Mackerras, PowerPC email list, rmk+kernel
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'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 */
>
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-20 1:28 ` Benjamin Herrenschmidt
@ 2013-11-20 2:04 ` Li Zhong
2013-11-20 23:23 ` Russell King - ARM Linux
1 sibling, 0 replies; 9+ messages in thread
From: Li Zhong @ 2013-11-20 2:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, PowerPC email list, rmk+kernel
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 */
> >
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-20 1:28 ` Benjamin Herrenschmidt
2013-11-20 2:04 ` Li Zhong
@ 2013-11-20 23:23 ` Russell King - ARM Linux
2013-11-21 0:01 ` Benjamin Herrenschmidt
1 sibling, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2013-11-20 23:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, PowerPC email list, Li Zhong
On Wed, Nov 20, 2013 at 12:28:02PM +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'm asking because I'm worried about breakage elsewhere...
I'd expect that the reason it doesn't work is that the dma_set_mask()
is failing, which means we don't go on to set the coherent mask.
Li Zong's patch works around the issue of a failing dma_set_mask(),
but as I've already said elsewhere, the real fix is to get whatever
created the struct device to initialise the dev->dma_mask with a
bus default.
Using dma_coerce_xxx() merely makes the problem "go away" papering
over the issue - it's fine to do it this way, but someone should still
fix the broken code creating these devices...
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-20 23:23 ` Russell King - ARM Linux
@ 2013-11-21 0:01 ` Benjamin Herrenschmidt
2013-11-21 0:08 ` Russell King - ARM Linux
0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2013-11-21 0:01 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Paul Mackerras, PowerPC email list, Li Zhong
On Wed, 2013-11-20 at 23:23 +0000, Russell King - ARM Linux wrote:
> Li Zong's patch works around the issue of a failing dma_set_mask(),
> but as I've already said elsewhere, the real fix is to get whatever
> created the struct device to initialise the dev->dma_mask with a
> bus default.
>
> Using dma_coerce_xxx() merely makes the problem "go away" papering
> over the issue - it's fine to do it this way, but someone should still
> fix the broken code creating these devices...
Ok, they are created by the vio bus core, so it should be doing the
job here of setting the dma_mask pointer to a proper value.
Li, can you take care of that ? Look at other bus types we have in
there such as the macio bus etc...
Cheers,
Ben.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-21 0:01 ` Benjamin Herrenschmidt
@ 2013-11-21 0:08 ` Russell King - ARM Linux
2013-11-21 0:22 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2013-11-21 0:08 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, PowerPC email list, Li Zhong
On Thu, Nov 21, 2013 at 11:01:42AM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2013-11-20 at 23:23 +0000, Russell King - ARM Linux wrote:
> > Li Zong's patch works around the issue of a failing dma_set_mask(),
> > but as I've already said elsewhere, the real fix is to get whatever
> > created the struct device to initialise the dev->dma_mask with a
> > bus default.
> >
> > Using dma_coerce_xxx() merely makes the problem "go away" papering
> > over the issue - it's fine to do it this way, but someone should still
> > fix the broken code creating these devices...
>
> Ok, they are created by the vio bus core, so it should be doing the
> job here of setting the dma_mask pointer to a proper value.
>
> Li, can you take care of that ? Look at other bus types we have in
> there such as the macio bus etc...
Oh, hang on a moment, this is the "bus" code.
In which case, the question becomes: do vio devices ever need to have
a separate streaming DMA mask from a coherent DMA mask? If not, then
something like the following is what's needed here, and I should've
never have used dma_set_mask_and_coherent().
dma_set_mask_and_coherent() (and the other dma_set_mask() functions)
are really supposed to be used by drivers only.
arch/powerpc/kernel/vio.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
index e7d0c88f621a..d771778f398e 100644
--- a/arch/powerpc/kernel/vio.c
+++ b/arch/powerpc/kernel/vio.c
@@ -1419,7 +1419,8 @@ 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));
+ viodev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
+ viodev->dev.dma_mask = &viodev->dev.coherent_dma_mask;
}
/* register with generic device framework */
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-21 0:08 ` Russell King - ARM Linux
@ 2013-11-21 0:22 ` Benjamin Herrenschmidt
2013-11-21 1:56 ` Li Zhong
2013-11-28 9:22 ` [PATCH powerpc] Revert c6102609 and replace it with the correct fix for vio dma mask setting Li Zhong
0 siblings, 2 replies; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2013-11-21 0:22 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Paul Mackerras, PowerPC email list, Li Zhong
On Thu, 2013-11-21 at 00:08 +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 21, 2013 at 11:01:42AM +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2013-11-20 at 23:23 +0000, Russell King - ARM Linux wrote:
> > > Li Zong's patch works around the issue of a failing dma_set_mask(),
> > > but as I've already said elsewhere, the real fix is to get whatever
> > > created the struct device to initialise the dev->dma_mask with a
> > > bus default.
> > >
> > > Using dma_coerce_xxx() merely makes the problem "go away" papering
> > > over the issue - it's fine to do it this way, but someone should still
> > > fix the broken code creating these devices...
> >
> > Ok, they are created by the vio bus core, so it should be doing the
> > job here of setting the dma_mask pointer to a proper value.
> >
> > Li, can you take care of that ? Look at other bus types we have in
> > there such as the macio bus etc...
>
> Oh, hang on a moment, this is the "bus" code.
>
> In which case, the question becomes: do vio devices ever need to have
> a separate streaming DMA mask from a coherent DMA mask? If not, then
> something like the following is what's needed here, and I should've
> never have used dma_set_mask_and_coherent().
No, a single mask.
> dma_set_mask_and_coherent() (and the other dma_set_mask() functions)
> are really supposed to be used by drivers only.
>
> arch/powerpc/kernel/vio.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
> index e7d0c88f621a..d771778f398e 100644
> --- a/arch/powerpc/kernel/vio.c
> +++ b/arch/powerpc/kernel/vio.c
> @@ -1419,7 +1419,8 @@ 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));
> + viodev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> + viodev->dev.dma_mask = &viodev->dev.coherent_dma_mask;
> }
>
> /* register with generic device framework */
Right that's exactly what I had in mind. Li, can you test this please ?
The previous "fix" using dma_coerce_mask_and_coherent() is already on
its way to Linus, so we'll rework the above patch to undo it but for
now please test.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
2013-11-21 0:22 ` Benjamin Herrenschmidt
@ 2013-11-21 1:56 ` Li Zhong
2013-11-28 9:22 ` [PATCH powerpc] Revert c6102609 and replace it with the correct fix for vio dma mask setting Li Zhong
1 sibling, 0 replies; 9+ messages in thread
From: Li Zhong @ 2013-11-21 1:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: PowerPC email list, Russell King - ARM Linux, Paul Mackerras
On Thu, 2013-11-21 at 11:22 +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2013-11-21 at 00:08 +0000, Russell King - ARM Linux wrote:
> > On Thu, Nov 21, 2013 at 11:01:42AM +1100, Benjamin Herrenschmidt wrote:
> > > On Wed, 2013-11-20 at 23:23 +0000, Russell King - ARM Linux wrote:
> > > > Li Zong's patch works around the issue of a failing dma_set_mask(),
> > > > but as I've already said elsewhere, the real fix is to get whatever
> > > > created the struct device to initialise the dev->dma_mask with a
> > > > bus default.
> > > >
> > > > Using dma_coerce_xxx() merely makes the problem "go away" papering
> > > > over the issue - it's fine to do it this way, but someone should still
> > > > fix the broken code creating these devices...
> > >
> > > Ok, they are created by the vio bus core, so it should be doing the
> > > job here of setting the dma_mask pointer to a proper value.
> > >
> > > Li, can you take care of that ? Look at other bus types we have in
> > > there such as the macio bus etc...
> >
> > Oh, hang on a moment, this is the "bus" code.
> >
> > In which case, the question becomes: do vio devices ever need to have
> > a separate streaming DMA mask from a coherent DMA mask? If not, then
> > something like the following is what's needed here, and I should've
> > never have used dma_set_mask_and_coherent().
>
> No, a single mask.
>
> > dma_set_mask_and_coherent() (and the other dma_set_mask() functions)
> > are really supposed to be used by drivers only.
> >
> > arch/powerpc/kernel/vio.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
> > index e7d0c88f621a..d771778f398e 100644
> > --- a/arch/powerpc/kernel/vio.c
> > +++ b/arch/powerpc/kernel/vio.c
> > @@ -1419,7 +1419,8 @@ 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));
> > + viodev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
> > + viodev->dev.dma_mask = &viodev->dev.coherent_dma_mask;
> > }
> >
> > /* register with generic device framework */
>
> Right that's exactly what I had in mind. Li, can you test this please ?
Sure, and it works.
Tested-by: Li Zhong <zhong@linux.vnet.ibm.com>
>
> The previous "fix" using dma_coerce_mask_and_coherent() is already on
> its way to Linus, so we'll rework the above patch to undo it but for
> now please test.
>
> Cheers,
> Ben.
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH powerpc] Revert c6102609 and replace it with the correct fix for vio dma mask setting
2013-11-21 0:22 ` Benjamin Herrenschmidt
2013-11-21 1:56 ` Li Zhong
@ 2013-11-28 9:22 ` Li Zhong
1 sibling, 0 replies; 9+ messages in thread
From: Li Zhong @ 2013-11-28 9:22 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: PowerPC email list, Russell King - ARM Linux, Paul Mackerras
This patch reverts my previous "fix", and replace it with the correct
fix from Russell.
And as Russell pointed out -- dma_set_mask_and_coherent() (and the other
dma_set_mask() functions) are really supposed to be used by drivers
only.
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
arch/powerpc/kernel/vio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
index 76a6482..d771778 100644
--- a/arch/powerpc/kernel/vio.c
+++ b/arch/powerpc/kernel/vio.c
@@ -1419,7 +1419,8 @@ 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_coerce_mask_and_coherent(&viodev->dev, DMA_BIT_MASK(64));
+ viodev->dev.coherent_dma_mask = DMA_BIT_MASK(64);
+ viodev->dev.dma_mask = &viodev->dev.coherent_dma_mask;
}
/* register with generic device framework */
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-27 9:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-19 8:11 [RFC PATCH powerpc] Fix a dma_mask issue of vio Li Zhong
2013-11-20 1:28 ` Benjamin Herrenschmidt
2013-11-20 2:04 ` Li Zhong
2013-11-20 23:23 ` Russell King - ARM Linux
2013-11-21 0:01 ` Benjamin Herrenschmidt
2013-11-21 0:08 ` Russell King - ARM Linux
2013-11-21 0:22 ` Benjamin Herrenschmidt
2013-11-21 1:56 ` Li Zhong
2013-11-28 9:22 ` [PATCH powerpc] Revert c6102609 and replace it with the correct fix for vio dma mask setting Li Zhong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).