Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
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 3015: couldn't initialize event pool [5.688822] ibmvscsi: probe of 3015 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... ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
On Wed, 2013-11-20 at 23:23 +, 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
On Thu, Nov 21, 2013 at 11:01:42AM +1100, Benjamin Herrenschmidt wrote: On Wed, 2013-11-20 at 23:23 +, 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 */ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
On Thu, 2013-11-21 at 00:08 +, 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 +, 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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
On Thu, 2013-11-21 at 11:22 +1100, Benjamin Herrenschmidt wrote: On Thu, 2013-11-21 at 00:08 +, 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 +, 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 zh...@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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
On Tue, 2013-11-19 at 16:11 +0800, Li Zhong wrote: I encountered following issue: [0.283035] ibmvscsi 3015: couldn't initialize event pool [5.688822] ibmvscsi: probe of 3015 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 */ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH powerpc] Fix a dma_mask issue of vio
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 3015: couldn't initialize event pool
[5.688822] ibmvscsi: probe of 3015 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 */
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
