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(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2013-11-20 01:28:36
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().
quoted
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.
On Wed, 2013-11-20 at 12:28 +1100, Benjamin Herrenschmidt wrote:
On Tue, 2013-11-19 at 16:11 +0800, Li Zhong wrote:
quoted
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().
quoted
quoted
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.
From: Russell King - ARM Linux <hidden> Date: 2013-11-20 23:24:04
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:
quoted
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().
quoted
quoted
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...
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2013-11-21 00:02:08
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.
From: Russell King - ARM Linux <hidden> Date: 2013-11-21 00:08:33
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:
quoted
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(-)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2013-11-21 00:22:43
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:
quoted
On Wed, 2013-11-20 at 23:23 +0000, Russell King - ARM Linux wrote:
quoted
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.
quoted hunk
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(-)
@@ -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,incasedriverdoesn'tsetitexplicitly*/-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.
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:
quoted
On Thu, Nov 21, 2013 at 11:01:42AM +1100, Benjamin Herrenschmidt wrote:
quoted
On Wed, 2013-11-20 at 23:23 +0000, Russell King - ARM Linux wrote:
quoted
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.
quoted
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(-)
@@ -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,incasedriverdoesn'tsetitexplicitly*/-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 <redacted>
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.
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 <redacted>
---
arch/powerpc/kernel/vio.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)