On Tue, 2023-09-12 at 11:24 +0200, Uwe Kleine-König wrote:
> >
> > The newly added "struct u64_stats_sync syncp" uses a seqlock
> > internally, which is broken into multiple words on 32bit machines, and
> > needs to be initialized properly. You need to call u64_stats_init on
> > syncp before first usage.
>
> This is done. The problematic thing is that in stmmac_open() ->
> __stmmac_open() the syncp initialized before is overwritten by
>
> memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
>
> Do I need to point out that this is ugly?
I think it also leaks the (lockdep) state since it reinits the syncp
(and a lot of other state) doing this. This is also called when the MTU
changes.
Also, I couldn't convince myself that it's even race-free? Even if it
is, it's not really obvious, IMHO.
So it seems to me that really this needs to be split into data that
actually should be reinitialized, and data that shouldn't, or just not
use memcpy() here but copy only the relevant state?
But anyway, I have no skin in this game - just reviewing this because I
was trying to help out Uwe.
johannes
On Tue, Sep 12, 2023 at 11:30:14AM +0200, Johannes Berg wrote:
> On Tue, 2023-09-12 at 11:24 +0200, Uwe Kleine-König wrote:
> > >
> > > The newly added "struct u64_stats_sync syncp" uses a seqlock
> > > internally, which is broken into multiple words on 32bit machines, and
> > > needs to be initialized properly. You need to call u64_stats_init on
> > > syncp before first usage.
> >
> > This is done. The problematic thing is that in stmmac_open() ->
> > __stmmac_open() the syncp initialized before is overwritten by
> >
> > memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
Thank Johannes and Uwe for pointing out the issue.
> >
> > Do I need to point out that this is ugly?
>
> I think it also leaks the (lockdep) state since it reinits the syncp
> (and a lot of other state) doing this. This is also called when the MTU
> changes.
>
> Also, I couldn't convince myself that it's even race-free? Even if it
> is, it's not really obvious, IMHO.
>
> So it seems to me that really this needs to be split into data that
> actually should be reinitialized, and data that shouldn't, or just not
> use memcpy() here but copy only the relevant state?
Since we are in rc1, I need to fix the bug with as small changes as
possible. so another solution could be: replace rx/tx stats structure
with pointers, then setup pointers in the new allocated dma_conf with
the old one as current code did for dma_tx_size/dma_rx_size in
stmmac_setup_dma_desc():
dma_conf->dma_tx_size = priv->dma_conf.dma_tx_size
Is it acceptable?
Thanks
>
> But anyway, I have no skin in this game - just reviewing this because I
> was trying to help out Uwe.
>
> johannes
On Wed, 2023-09-13 at 22:34 +0800, Jisheng Zhang wrote:
>
> Since we are in rc1, I need to fix the bug with as small changes as
> possible. so another solution could be: replace rx/tx stats structure
> with pointers, then setup pointers in the new allocated dma_conf with
> the old one as current code did for dma_tx_size/dma_rx_size in
> stmmac_setup_dma_desc():
>
> dma_conf->dma_tx_size = priv->dma_conf.dma_tx_size
>
> Is it acceptable?
I'm not sure who you're asking of all the people in this thread, but I
honestly don't understand much about this driver other than what I
gleaned in the few minutes looking at it with Uwe ... so I don't think
I'm able to answer that question :)
You could also just move the stats out of the structure entirely, I
guess? And perhaps even start some new structure where more things might
move in the future that shouldn't be reallocated?
I'm all for not moving things now that haven't caused problems, but I
guess doing a minimal fix just for the sake of being minimal in -rc1
wouldn't be my preference.
johannes
Hello,
On Wed, Sep 13, 2023 at 10:34:42PM +0800, Jisheng Zhang wrote:
> On Tue, Sep 12, 2023 at 11:30:14AM +0200, Johannes Berg wrote:
> > On Tue, 2023-09-12 at 11:24 +0200, Uwe Kleine-K?nig wrote:
> > > >
> > > > The newly added "struct u64_stats_sync syncp" uses a seqlock
> > > > internally, which is broken into multiple words on 32bit machines, and
> > > > needs to be initialized properly. You need to call u64_stats_init on
> > > > syncp before first usage.
> > >
> > > This is done. The problematic thing is that in stmmac_open() ->
> > > __stmmac_open() the syncp initialized before is overwritten by
> > >
> > > memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));
>
> Thank Johannes and Uwe for pointing out the issue.
>
> > >
> > > Do I need to point out that this is ugly?
> >
> > I think it also leaks the (lockdep) state since it reinits the syncp
> > (and a lot of other state) doing this. This is also called when the MTU
> > changes.
> >
> > Also, I couldn't convince myself that it's even race-free? Even if it
> > is, it's not really obvious, IMHO.
> >
> > So it seems to me that really this needs to be split into data that
> > actually should be reinitialized, and data that shouldn't, or just not
> > use memcpy() here but copy only the relevant state?
>
> Since we are in rc1, I need to fix the bug with as small changes as
> possible. so another solution could be: replace rx/tx stats structure
> with pointers, then setup pointers in the new allocated dma_conf with
> the old one as current code did for dma_tx_size/dma_rx_size in
> stmmac_setup_dma_desc():
>
> dma_conf->dma_tx_size = priv->dma_conf.dma_tx_size
>
> Is it acceptable?
I wondered if you can just initialize the data directly in *priv, instead
of setting up a local copy, initialize that one + copy it over?!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |