| Message ID | 20200323014526.3340884-6-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | 24891dd8d40d71c034023d2a037c97df1714393b |
| Headers | show |
| Series | [1/6] net: dwc_eth_qos: Fully rewrite RX descriptor field 3 | expand |
On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: > > The DMA may attempt to write a DMA descriptor in the ring while it is > being updated. By writing the DMA descriptor buffer address to 0, it > is assured the DMA will not use such a buffer and the buffer can be > updated without any interference. > > Signed-off-by: Marek Vasut <marex at denx.de> > Cc: Joe Hershberger <joe.hershberger at ni.com> > Cc: Patrice Chotard <patrice.chotard at st.com> > Cc: Patrick Delaunay <patrick.delaunay at st.com> > Cc: Ramon Fried <rfried.dev at gmail.com> > Cc: Stephen Warren <swarren at nvidia.com> > --- > drivers/net/dwc_eth_qos.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > index 7dadb10fe7..c86b9d59a5 100644 > --- a/drivers/net/dwc_eth_qos.c > +++ b/drivers/net/dwc_eth_qos.c > @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) > > rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); > > + rx_desc->des0 = 0; > + mb(); Better use wmb() for better understanding your goal here ? > + eqos->config->ops->eqos_flush_desc(rx_desc); > eqos->config->ops->eqos_inval_buffer(packet, length); > - > rx_desc->des0 = (u32)(ulong)packet; > rx_desc->des1 = 0; > rx_desc->des2 = 0; > -- > 2.25.1 >
On 3/23/20 8:09 AM, Ramon Fried wrote: > On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: >> >> The DMA may attempt to write a DMA descriptor in the ring while it is >> being updated. By writing the DMA descriptor buffer address to 0, it >> is assured the DMA will not use such a buffer and the buffer can be >> updated without any interference. >> >> Signed-off-by: Marek Vasut <marex at denx.de> >> Cc: Joe Hershberger <joe.hershberger at ni.com> >> Cc: Patrice Chotard <patrice.chotard at st.com> >> Cc: Patrick Delaunay <patrick.delaunay at st.com> >> Cc: Ramon Fried <rfried.dev at gmail.com> >> Cc: Stephen Warren <swarren at nvidia.com> >> --- >> drivers/net/dwc_eth_qos.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c >> index 7dadb10fe7..c86b9d59a5 100644 >> --- a/drivers/net/dwc_eth_qos.c >> +++ b/drivers/net/dwc_eth_qos.c >> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) >> >> rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); >> >> + rx_desc->des0 = 0; >> + mb(); > Better use wmb() for better understanding your goal here ? The driver uses mb() all over the place, so I'm just keeping it consistent here. But I wonder whether we even need all those barriers in this driver at all.
On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex at denx.de> wrote: > > On 3/23/20 8:09 AM, Ramon Fried wrote: > > On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: > >> > >> The DMA may attempt to write a DMA descriptor in the ring while it is > >> being updated. By writing the DMA descriptor buffer address to 0, it > >> is assured the DMA will not use such a buffer and the buffer can be > >> updated without any interference. > >> > >> Signed-off-by: Marek Vasut <marex at denx.de> > >> Cc: Joe Hershberger <joe.hershberger at ni.com> > >> Cc: Patrice Chotard <patrice.chotard at st.com> > >> Cc: Patrick Delaunay <patrick.delaunay at st.com> > >> Cc: Ramon Fried <rfried.dev at gmail.com> > >> Cc: Stephen Warren <swarren at nvidia.com> > >> --- > >> drivers/net/dwc_eth_qos.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > >> index 7dadb10fe7..c86b9d59a5 100644 > >> --- a/drivers/net/dwc_eth_qos.c > >> +++ b/drivers/net/dwc_eth_qos.c > >> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) > >> > >> rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); > >> > >> + rx_desc->des0 = 0; > >> + mb(); > > Better use wmb() for better understanding your goal here ? > > The driver uses mb() all over the place, so I'm just keeping it > consistent here. > > But I wonder whether we even need all those barriers in this driver at all. In that case that's fine :)
On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: > > The DMA may attempt to write a DMA descriptor in the ring while it is > being updated. By writing the DMA descriptor buffer address to 0, it > is assured the DMA will not use such a buffer and the buffer can be > updated without any interference. > > Signed-off-by: Marek Vasut <marex at denx.de> > Cc: Joe Hershberger <joe.hershberger at ni.com> > Cc: Patrice Chotard <patrice.chotard at st.com> > Cc: Patrick Delaunay <patrick.delaunay at st.com> > Cc: Ramon Fried <rfried.dev at gmail.com> > Cc: Stephen Warren <swarren at nvidia.com> > --- > drivers/net/dwc_eth_qos.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > index 7dadb10fe7..c86b9d59a5 100644 > --- a/drivers/net/dwc_eth_qos.c > +++ b/drivers/net/dwc_eth_qos.c > @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) > > rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); > > + rx_desc->des0 = 0; > + mb(); > + eqos->config->ops->eqos_flush_desc(rx_desc); > eqos->config->ops->eqos_inval_buffer(packet, length); > - > rx_desc->des0 = (u32)(ulong)packet; > rx_desc->des1 = 0; > rx_desc->des2 = 0; > -- > 2.25.1 > Reviewed-by: Ramon Fried <rfried.dev at gmail.com>
On 3/23/20 1:19 PM, Ramon Fried wrote: > On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex at denx.de> wrote: >> >> On 3/23/20 8:09 AM, Ramon Fried wrote: >>> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: >>>> >>>> The DMA may attempt to write a DMA descriptor in the ring while it is >>>> being updated. By writing the DMA descriptor buffer address to 0, it >>>> is assured the DMA will not use such a buffer and the buffer can be >>>> updated without any interference. >>>> >>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>> Cc: Joe Hershberger <joe.hershberger at ni.com> >>>> Cc: Patrice Chotard <patrice.chotard at st.com> >>>> Cc: Patrick Delaunay <patrick.delaunay at st.com> >>>> Cc: Ramon Fried <rfried.dev at gmail.com> >>>> Cc: Stephen Warren <swarren at nvidia.com> >>>> --- >>>> drivers/net/dwc_eth_qos.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c >>>> index 7dadb10fe7..c86b9d59a5 100644 >>>> --- a/drivers/net/dwc_eth_qos.c >>>> +++ b/drivers/net/dwc_eth_qos.c >>>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) >>>> >>>> rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); >>>> >>>> + rx_desc->des0 = 0; >>>> + mb(); >>> Better use wmb() for better understanding your goal here ? >> >> The driver uses mb() all over the place, so I'm just keeping it >> consistent here. >> >> But I wonder whether we even need all those barriers in this driver at all. > In that case that's fine :) I mean, I am asking about the need for those barriers here, that's my question here.
On Mon, Mar 23, 2020 at 2:22 PM Marek Vasut <marex at denx.de> wrote: > > On 3/23/20 1:19 PM, Ramon Fried wrote: > > On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex at denx.de> wrote: > >> > >> On 3/23/20 8:09 AM, Ramon Fried wrote: > >>> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: > >>>> > >>>> The DMA may attempt to write a DMA descriptor in the ring while it is > >>>> being updated. By writing the DMA descriptor buffer address to 0, it > >>>> is assured the DMA will not use such a buffer and the buffer can be > >>>> updated without any interference. > >>>> > >>>> Signed-off-by: Marek Vasut <marex at denx.de> > >>>> Cc: Joe Hershberger <joe.hershberger at ni.com> > >>>> Cc: Patrice Chotard <patrice.chotard at st.com> > >>>> Cc: Patrick Delaunay <patrick.delaunay at st.com> > >>>> Cc: Ramon Fried <rfried.dev at gmail.com> > >>>> Cc: Stephen Warren <swarren at nvidia.com> > >>>> --- > >>>> drivers/net/dwc_eth_qos.c | 4 +++- > >>>> 1 file changed, 3 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c > >>>> index 7dadb10fe7..c86b9d59a5 100644 > >>>> --- a/drivers/net/dwc_eth_qos.c > >>>> +++ b/drivers/net/dwc_eth_qos.c > >>>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) > >>>> > >>>> rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); > >>>> > >>>> + rx_desc->des0 = 0; > >>>> + mb(); > >>> Better use wmb() for better understanding your goal here ? > >> > >> The driver uses mb() all over the place, so I'm just keeping it > >> consistent here. > >> > >> But I wonder whether we even need all those barriers in this driver at all. > > In that case that's fine :) > > I mean, I am asking about the need for those barriers here, that's my > question here. Better safe than sorry. unless you have the means to test it to see if it doesn't break. Thanks, Ramon.
On 3/23/20 1:50 PM, Ramon Fried wrote: > On Mon, Mar 23, 2020 at 2:22 PM Marek Vasut <marex at denx.de> wrote: >> >> On 3/23/20 1:19 PM, Ramon Fried wrote: >>> On Mon, Mar 23, 2020 at 2:00 PM Marek Vasut <marex at denx.de> wrote: >>>> >>>> On 3/23/20 8:09 AM, Ramon Fried wrote: >>>>> On Mon, Mar 23, 2020 at 3:45 AM Marek Vasut <marex at denx.de> wrote: >>>>>> >>>>>> The DMA may attempt to write a DMA descriptor in the ring while it is >>>>>> being updated. By writing the DMA descriptor buffer address to 0, it >>>>>> is assured the DMA will not use such a buffer and the buffer can be >>>>>> updated without any interference. >>>>>> >>>>>> Signed-off-by: Marek Vasut <marex at denx.de> >>>>>> Cc: Joe Hershberger <joe.hershberger at ni.com> >>>>>> Cc: Patrice Chotard <patrice.chotard at st.com> >>>>>> Cc: Patrick Delaunay <patrick.delaunay at st.com> >>>>>> Cc: Ramon Fried <rfried.dev at gmail.com> >>>>>> Cc: Stephen Warren <swarren at nvidia.com> >>>>>> --- >>>>>> drivers/net/dwc_eth_qos.c | 4 +++- >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c >>>>>> index 7dadb10fe7..c86b9d59a5 100644 >>>>>> --- a/drivers/net/dwc_eth_qos.c >>>>>> +++ b/drivers/net/dwc_eth_qos.c >>>>>> @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) >>>>>> >>>>>> rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); >>>>>> >>>>>> + rx_desc->des0 = 0; >>>>>> + mb(); >>>>> Better use wmb() for better understanding your goal here ? >>>> >>>> The driver uses mb() all over the place, so I'm just keeping it >>>> consistent here. >>>> >>>> But I wonder whether we even need all those barriers in this driver at all. >>> In that case that's fine :) >> >> I mean, I am asking about the need for those barriers here, that's my >> question here. > Better safe than sorry. unless you have the means to test it to see if > it doesn't break. On STM32MP1, yes. Not on the Tegra platform.
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index 7dadb10fe7..c86b9d59a5 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -1431,8 +1431,10 @@ static int eqos_free_pkt(struct udevice *dev, uchar *packet, int length) rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx]); + rx_desc->des0 = 0; + mb(); + eqos->config->ops->eqos_flush_desc(rx_desc); eqos->config->ops->eqos_inval_buffer(packet, length); - rx_desc->des0 = (u32)(ulong)packet; rx_desc->des1 = 0; rx_desc->des2 = 0;
The DMA may attempt to write a DMA descriptor in the ring while it is being updated. By writing the DMA descriptor buffer address to 0, it is assured the DMA will not use such a buffer and the buffer can be updated without any interference. Signed-off-by: Marek Vasut <marex at denx.de> Cc: Joe Hershberger <joe.hershberger at ni.com> Cc: Patrice Chotard <patrice.chotard at st.com> Cc: Patrick Delaunay <patrick.delaunay at st.com> Cc: Ramon Fried <rfried.dev at gmail.com> Cc: Stephen Warren <swarren at nvidia.com> --- drivers/net/dwc_eth_qos.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)