On Tue, 2020-07-14 at 14:52 +1000, Alexey Kardashevskiy wrote:
>
> On 14/07/2020 12:40, Leonardo Bras wrote:
> > Thank you for this feedback Alexey!
> >
> > On Mon, 2020-07-13 at 17:33 +1000, Alexey Kardashevskiy wrote:
> > > [...]
> > > > - int len, ret;
> > > > + int len, ret, reset_win_ext;
> > >
> > > Make it "reset_token".
> >
> > Oh, it's not a token here, it just checks if the reset_win extension
> > exists. The token would be returned in *value, but since we did not
> > need it here, it's not copied.
>
> ah right, so it is a bool actually.
In fact I did it a int, as it's the return value of ddw_read_ext(),
which can return 0 on success and -error otherwise.
> > > > [...]
> > > > -out_failed:
> > > > +out_restore_defwin:
> > > > + if (default_win && reset_win_ext == 0)
> > >
> > > reset_win_ext potentially may be uninitialized here. Yeah I know it is
> > > tied to default_win but still.
> >
> > I can't see it being used uninitialized here, as you said it's tied to
> > default_win.
>
> Where it is declared - it is not initialized so in theory it can skip
> "if (query.windows_available == 0)".
Humm, I thought doing if (default_win && reset_win_ext == 0) would
guarantee default_win to be tested before reset_win_ext is ever tested,
so I could control it using default_win.
>
>
> > Could you please tell me how it can be used uninitialized here, or what
> > is bad by doing this way?
> >
> > > After looking at this function for a few minutes, it could use some
> > > refactoring (way too many gotos) such as:
> >
> > Yes, I agree.
> >
> > > 1. move (query.page_size & xx) checks before "if
> > > (query.windows_available == 0)"
> >
> > Moving 'page_size selection' above 'checking windows available' will
> > need us to duplicate the 'page_size selection' after the new query,
> > inside the if.
>
> page_size selection is not going to change, why?
In theory, a query after freeing the default DMA window could have a
different (bigger) page size, so we should test again.
>
>
> > I mean, as query will be done again, it will need to get the (new) page
> > size.
> >
> > > 2. move "win64 = kzalloc(sizeof(struct property), GFP_KERNEL)" before
> > > "if (query.windows_available == 0)"
> > > 3. call "reset_dma_window(dev, pdn)" inside the "if
> > > (query.windows_available == 0)" branch.
> > > Then you can drop all "goto out_restore_defwin" and move default_win and
> > > reset_win_ext inside "if (query.windows_available == 0)".
> >
> > I did all changes suggested locally and did some analysis in the
> > result:
> >
> > I did not see a way to put default_win and reset_win_ext inside
> > "if (query.windows_available == 0)", because if we still need a way to
> > know if the default window was removed, and if so, restore in case
> > anything ever fails ahead (like creating the node property).
>
> Ah, I missed that new out_restore_defwin label is between other exit
> labels. Sorry :-/
>
>
> > reset_win_ext = ddw_read_ext(pdn,
> > DDW_EXT_RESET_DMA_WIN, NULL);
> > - if (reset_win_ext)
> > + if (reset_win_ext){
> > + default_win = NULL;
> > goto out_failed;
> > + }
>
> This says "if we can reset, then we fail", no?
Here ddw_read_ext() should return 0 if extension was found, and
(-EINVAL, -ENODATA or -EOVERFLOW) otherwise.
So it should return nonzero if we can't find the extension, in which
case we should fail.
>
> > remove_dma_window(pdn, ddw_avail, default_win);
>
> I think you can do "default_win=NULL" here and later at
> out_restore_defwin check if it is NULL - then call reset.
Currently I initialize 'default_win = NULL', and it only changes when I
read the default DMA window. If reset is not available I restore it to
NULL, so it will be not-NULL only when the have removed the default DMA
window.
If I make it NULL here, we either never reset the default DMA window
(as it is now "if (default_win)" ) or we may always reset it (in case
"if (default_win == NULL)").
If you think it's better, I can create a bool variable like
"default_win_removed", initialized with 'false', which can be assigned
here with 'true' and test in the end if(default_win_removed) reset();
This would allow to move default_win inside this 'if block'.
What do you think?
> > [...]
> >
> > -out_restore_defwin:
> > - if (default_win && reset_win_ext == 0)
> > +out_failed:
> > + if (default_win)
> > reset_dma_window(dev, pdn);
> >
> > -out_failed:
> > fpdn = kzalloc(sizeof(*fpdn), GFP_KERNEL);
> > if (!fpdn)
> > goto out_unlock;
> >
> > #####
> >
> > What do you think?
> >
> >
> >
> > > The rest of the series is good as it is,
> >
> > Thank you :)
> >
> > > however it may conflict with
> > > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200713062348.100552-1-...@ozlabs.ru/
> > > and the patchset it is made on top of -
> > > https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=188385 .
> >
> > <From the message of the first link>
> > > (do not rush, let me finish reviewing this first)
> >
> > Ok, I have no problem rebasing on top of those patchsets, but what
> > would you suggest to be done?
>
> Polish this patch one more time and if by the time when you reposted it
> the other patchset is not in upstream, I'll ask Michael to take yours first.
Ok :)
>
> > Would it be ok doing a big multi-author patchset, so we guarantee it
> > being applied in the correct order?
> > > (You probably want me to rebase my patchset on top of Hellwig + yours,
> > right?)
>
> Nah, at least not yet.
Thank you!
