It seems that mxic_nfc_probe() missed invocation of
mxic_nfc_clk_enable(). The patch fixed that. In addition, error handling
was refined appropriately.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Evgeny Novikov <[email protected]>
Co-developed-by: Kirill Shilimanov <[email protected]>
Signed-off-by: Kirill Shilimanov <[email protected]>
---
drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_nand.c
index da1070993994..37e75bf60ee5 100644
--- a/drivers/mtd/nand/raw/mxic_nand.c
+++ b/drivers/mtd/nand/raw/mxic_nand.c
@@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct platform_device *pdev)
if (IS_ERR(nfc->send_dly_clk))
return PTR_ERR(nfc->send_dly_clk);
+ err = mxic_nfc_clk_enable(nfc);
+ if (err)
+ return err;
+
nfc->regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(nfc->regs))
- return PTR_ERR(nfc->regs);
+ if (IS_ERR(nfc->regs)) {
+ err = PTR_ERR(nfc->regs);
+ goto fail;
+ }
nand_chip = &nfc->chip;
mtd = nand_to_mtd(nand_chip);
@@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct platform_device *pdev)
nand_chip->controller = &nfc->controller;
irq = platform_get_irq(pdev, 0);
- if (irq < 0)
- return irq;
+ if (irq < 0) {
+ err = irq;
+ goto fail;
+ }
mxic_nfc_hw_init(nfc);
--
2.26.2
Hi Andy,
Andy Shevchenko <[email protected]> wrote on Thu, 12 Aug 2021
15:13:10 +0300:
> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]> wrote:
>
> > It seems that mxic_nfc_probe() missed invocation of
> > mxic_nfc_clk_enable(). The patch fixed that. In addition, error handling
> > was refined appropriately.
>
>
> NAK. Until you provide a deeper analysis, like how this works before your
> change.
>
>
> Please, don’t blindly generate patches, this can even your bot do, just
> think about each change and preferable test on the real hardware.
>
> The above is to all your lovely contributions.
>
>
> >
> > Found by Linux Driver Verification project (linuxtesting.org).
> >
> > Signed-off-by: Evgeny Novikov <[email protected]>
> > Co-developed-by: Kirill Shilimanov <[email protected]>
> > Signed-off-by: Kirill Shilimanov <[email protected]>
> > ---
> > drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_
> > nand.c
> > index da1070993994..37e75bf60ee5 100644
> > --- a/drivers/mtd/nand/raw/mxic_nand.c
> > +++ b/drivers/mtd/nand/raw/mxic_nand.c
> > @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct platform_device
> > *pdev)
> > if (IS_ERR(nfc->send_dly_clk))
> > return PTR_ERR(nfc->send_dly_clk);
> >
> > + err = mxic_nfc_clk_enable(nfc);
> > + if (err)
> > + return err;
As Andy said, this is not needed.
> > +
> > nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > - if (IS_ERR(nfc->regs))
> > - return PTR_ERR(nfc->regs);
> > + if (IS_ERR(nfc->regs)) {
> > + err = PTR_ERR(nfc->regs);
> > + goto fail;
> > + }
> >
> > nand_chip = &nfc->chip;
> > mtd = nand_to_mtd(nand_chip);
> > @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct platform_device
> > *pdev)
> > nand_chip->controller = &nfc->controller;
> >
> > irq = platform_get_irq(pdev, 0);
> > - if (irq < 0)
> > - return irq;
> > + if (irq < 0) {
> > + err = irq;
> > + goto fail;
However some reworking is needed in the error path.
That goto statement should be renamed and devm_request_irq() should not
jump to it.
> > + }
> >
> > mxic_nfc_hw_init(nfc);
> >
> > --
> > 2.26.2
> >
> >
>
Thanks,
Miquèl
Hi Andy,
On 12.08.2021 15:13, Andy Shevchenko wrote:
>
>
> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]
> <mailto:[email protected]>> wrote:
>
> It seems that mxic_nfc_probe() missed invocation of
> mxic_nfc_clk_enable(). The patch fixed that. In addition, error
> handling
> was refined appropriately.
>
>
> NAK. Until you provide a deeper analysis, like how this works before
> your change.
>
>
> Please, don’t blindly generate patches, this can even your bot do,
> just think about each change and preferable test on the real hardware.
>
> The above is to all your lovely contributions.
I completely agree with you that testing and debugging on the real
hardware is indispensable since this can help to reason about both found
bugs and patches suggested. Nevertheless, there are several cases when
this does not work. The most obvious one is lack of appropriate devices
on the spot that is not an obstacle for static analysis.
My patches are based on results of static verification (software model
checking). In a nutshell, this approach allows analyzing target programs
more accurately in comparison with widely used static analysis tools.
But this is not for free. Usually it needs much more computational
resources to get something meaningful (in a general case the task is
undecidable). Modern computer systems solve this issue partially. Worse
is that thorough static analysis needs more or less complete and correct
models of environments for target programs. For instance, for loadable
kernel modules it is necessary to specify correct sequences of callback
invocations, initialize their arguments at least to some extent and
develop models of some vital functions like kzalloc(). Moreover, it is
necessary to specify requirements if one wants to search for something
special rather than NULL pointer dereferences. We were able to devote a
relatively small part of our project to development of environment
models and requirement specifications for verification of the Linux
kernel. Also, we spent not so much time for application of our framework
for open source device drivers. Nevertheless, this helped to find out
quite a lot of bugs many of which are tricky enough. If you are
interested in more details I can send you our last paper and presentation.
Most our bug reports were accepted by developers. Rarely they preferred
to fix bugs according to their needs and vision, that is especially the
case for considerable changes that do need appropriate hardware and
testing. Just a few our bug reports were ignored or rejected. In the
latter case developers often pointed out us what is wrong with our
current understanding and models of the device driver environment or
requirement specifications. We always welcome this feedback as it allows
us to refine the stuff and, thus, to avoid false alarms and invalid
patches. Some developers requested scripts used for finding reported
issues, but it is not easy to add or refer them in patches like, say,
for Coccinelle since there is a bunch of external files developed in
different domain-specific languages as well as a huge verification
framework, that nobody will include into the source tree of the Linux
kernel.
Regarding your claim. We do not have an appropriate hardware. As usual
this bug report was prepared on the base of static verification results
purely. If you want to see on a particular artifact I based my decision
on, I can share a link to a visualized violation witness provided by a
verification tool. We have not any bots since used verification tools do
not give any suggestions on the issue roots. Maybe in some cases it is
possible to generate patches automatically on the base of these
verification results like, say, Coccinelle does, but it is another big
work. Of course, it is necessary to test the driver and confirm that
there is an issue or reject the patch. As always the feedback is welcome.
If you are not gratified with my explanation it would be great if you
and other developers will suggest any ideas how to enhance the process
if you find this relevant.
Best regards,
Evgeny Novikov
>
> Found by Linux Driver Verification project (linuxtesting.org
> <http://linuxtesting.org>).
>
> Signed-off-by: Evgeny Novikov <[email protected]
> <mailto:[email protected]>>
> Co-developed-by: Kirill Shilimanov <[email protected]
> <mailto:[email protected]>>
> Signed-off-by: Kirill Shilimanov <[email protected]
> <mailto:[email protected]>>
> ---
> drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/mxic_nand.c
> b/drivers/mtd/nand/raw/mxic_nand.c
> index da1070993994..37e75bf60ee5 100644
> --- a/drivers/mtd/nand/raw/mxic_nand.c
> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct
> platform_device *pdev)
> if (IS_ERR(nfc->send_dly_clk))
> return PTR_ERR(nfc->send_dly_clk);
>
> + err = mxic_nfc_clk_enable(nfc);
> + if (err)
> + return err;
> +
> nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> - if (IS_ERR(nfc->regs))
> - return PTR_ERR(nfc->regs);
> + if (IS_ERR(nfc->regs)) {
> + err = PTR_ERR(nfc->regs);
> + goto fail;
> + }
>
> nand_chip = &nfc->chip;
> mtd = nand_to_mtd(nand_chip);
> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct
> platform_device *pdev)
> nand_chip->controller = &nfc->controller;
>
> irq = platform_get_irq(pdev, 0);
> - if (irq < 0)
> - return irq;
> + if (irq < 0) {
> + err = irq;
> + goto fail;
> + }
>
> mxic_nfc_hw_init(nfc);
>
> --
> 2.26.2
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi Miquel,
On 16.08.2021 11:01, Miquel Raynal wrote:
> Hi Andy,
>
> Andy Shevchenko <[email protected]> wrote on Thu, 12 Aug 2021
> 15:13:10 +0300:
>
>> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]> wrote:
>>
>>> It seems that mxic_nfc_probe() missed invocation of
>>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error handling
>>> was refined appropriately.
>>
>> NAK. Until you provide a deeper analysis, like how this works before your
>> change.
>>
>>
>> Please, don’t blindly generate patches, this can even your bot do, just
>> think about each change and preferable test on the real hardware.
>>
>> The above is to all your lovely contributions.
>>
>>
>>> Found by Linux Driver Verification project (linuxtesting.org).
>>>
>>> Signed-off-by: Evgeny Novikov <[email protected]>
>>> Co-developed-by: Kirill Shilimanov <[email protected]>
>>> Signed-off-by: Kirill Shilimanov <[email protected]>
>>> ---
>>> drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
>>> 1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_
>>> nand.c
>>> index da1070993994..37e75bf60ee5 100644
>>> --- a/drivers/mtd/nand/raw/mxic_nand.c
>>> +++ b/drivers/mtd/nand/raw/mxic_nand.c
>>> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct platform_device
>>> *pdev)
>>> if (IS_ERR(nfc->send_dly_clk))
>>> return PTR_ERR(nfc->send_dly_clk);
>>>
>>> + err = mxic_nfc_clk_enable(nfc);
>>> + if (err)
>>> + return err;
> As Andy said, this is not needed.
>
>>> +
>>> nfc->regs = devm_platform_ioremap_resource(pdev, 0);
>>> - if (IS_ERR(nfc->regs))
>>> - return PTR_ERR(nfc->regs);
>>> + if (IS_ERR(nfc->regs)) {
>>> + err = PTR_ERR(nfc->regs);
>>> + goto fail;
>>> + }
>>>
>>> nand_chip = &nfc->chip;
>>> mtd = nand_to_mtd(nand_chip);
>>> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct platform_device
>>> *pdev)
>>> nand_chip->controller = &nfc->controller;
>>>
>>> irq = platform_get_irq(pdev, 0);
>>> - if (irq < 0)
>>> - return irq;
>>> + if (irq < 0) {
>>> + err = irq;
>>> + goto fail;
> However some reworking is needed in the error path.
>
> That goto statement should be renamed and devm_request_irq() should not
> jump to it.
>
We still need some help and clarification from those who are very
familiar with this sort of drivers or/and can test this particular
driver. mxic_nfc_clk_enable() is the complementary function for
mxic_nfc_clk_disable(). No other functions invoke
clk_prepare_enable()/clk_disable_unprepare() in the driver. Unlikely
somebody in its environment does that since driver specific clocks are
dealt with. At the moment the driver invokes mxic_nfc_clk_disable() on
error handling paths in probe, in remove and in mxic_nfc_set_freq().
mxic_nfc_clk_enable() is called just by mxic_nfc_set_freq() that
moreover does this after calling mxic_nfc_clk_disable() first. So, we
did not find any place in the driver that invokes mxic_nfc_clk_enable()
prior to mxic_nfc_clk_disable(). Basing on this we added
mxic_nfc_clk_enable() just after getting clocks. As I explained in the
previous large e-mail, we may be wrong in our understanding of the
driver environment or/and at specification of requirements being
checked. It would be great if you will point out on our mistakes.
Best regards,
Evgeny Novikov
>>> + }
>>>
>>> mxic_nfc_hw_init(nfc);
>>>
>>> --
>>> 2.26.2
>>>
>>>
> Thanks,
> Miquèl
Hi Evgeny,
+Mason from Macronix
Evgeny Novikov <[email protected]> wrote on Tue, 17 Aug 2021 13:36:03
+0300:
> Hi Miquel,
>
> On 16.08.2021 11:01, Miquel Raynal wrote:
> > Hi Andy,
> >
> > Andy Shevchenko <[email protected]> wrote on Thu, 12 Aug 2021
> > 15:13:10 +0300:
> >
> >> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]> wrote:
> >>
> >>> It seems that mxic_nfc_probe() missed invocation of
> >>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error handling
> >>> was refined appropriately.
> >>
> >> NAK. Until you provide a deeper analysis, like how this works before your
> >> change.
> >>
> >>
> >> Please, don’t blindly generate patches, this can even your bot do, just
> >> think about each change and preferable test on the real hardware.
> >>
> >> The above is to all your lovely contributions.
> >>
> >>
> >>> Found by Linux Driver Verification project (linuxtesting.org).
> >>>
> >>> Signed-off-by: Evgeny Novikov <[email protected]>
> >>> Co-developed-by: Kirill Shilimanov <[email protected]>
> >>> Signed-off-by: Kirill Shilimanov <[email protected]>
> >>> ---
> >>> drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> >>> 1 file changed, 12 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_
> >>> nand.c
> >>> index da1070993994..37e75bf60ee5 100644
> >>> --- a/drivers/mtd/nand/raw/mxic_nand.c
> >>> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> >>> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct platform_device
> >>> *pdev)
> >>> if (IS_ERR(nfc->send_dly_clk))
> >>> return PTR_ERR(nfc->send_dly_clk);
> >>>
> >>> + err = mxic_nfc_clk_enable(nfc);
> >>> + if (err)
> >>> + return err;
> > As Andy said, this is not needed.
> >
> >>> +
> >>> nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> >>> - if (IS_ERR(nfc->regs))
> >>> - return PTR_ERR(nfc->regs);
> >>> + if (IS_ERR(nfc->regs)) {
> >>> + err = PTR_ERR(nfc->regs);
> >>> + goto fail;
> >>> + }
> >>>
> >>> nand_chip = &nfc->chip;
> >>> mtd = nand_to_mtd(nand_chip);
> >>> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct platform_device
> >>> *pdev)
> >>> nand_chip->controller = &nfc->controller;
> >>>
> >>> irq = platform_get_irq(pdev, 0);
> >>> - if (irq < 0)
> >>> - return irq;
> >>> + if (irq < 0) {
> >>> + err = irq;
> >>> + goto fail;
> > However some reworking is needed in the error path.
> >
> > That goto statement should be renamed and devm_request_irq() should not
> > jump to it.
> >
>
> We still need some help and clarification from those who are very familiar with this sort of drivers or/and can test this particular driver. mxic_nfc_clk_enable() is the complementary function for mxic_nfc_clk_disable(). No other functions invoke clk_prepare_enable()/clk_disable_unprepare() in the driver. Unlikely somebody in its environment does that since driver specific clocks are dealt with. At the moment the driver invokes mxic_nfc_clk_disable() on error handling paths in probe, in remove and in mxic_nfc_set_freq(). mxic_nfc_clk_enable() is called just by mxic_nfc_set_freq() that moreover does this after calling mxic_nfc_clk_disable() first. So, we did not find any place in the driver that invokes mxic_nfc_clk_enable() prior to mxic_nfc_clk_disable(). Basing on this we added mxic_nfc_clk_enable() just after getting clocks. As I explained in the previous large e-mail, we may be wrong in our understanding of the driver environment or/and at specification of requirements being checked. It would be great if you will point out on our mistakes.
Enabling the clocks seems to only be needed to access the NAND device
and not the registers of the controller. Mason, is this statement
right? If this statement is wrong, then your proposal is not entirely
wrong in the sense that we must enable the missing clock *before*
accessing any register.
Otherwise for the two other clocks, we don't really care to enable them
before setting the actual frequency in ->setup_interface() (called by
nand_scan()) because at this point we don't yet know what timing mode
is best. Disabling the clock is not an issue even though it was not
enabled in the fist place anyway.
In all cases, the error path is wrong.
Thanks,
Miquèl
Miquel Raynal <[email protected]> wrote on Tue, 17 Aug 2021
12:55:21 +0200:
> Hi Evgeny,
>
> +Mason from Macronix
Mason's e-mail bounced, including YouChing in the discussion who may
also have an answer or perhaps knows who to include in the discussion
(see below for the context and question).
>
> Evgeny Novikov <[email protected]> wrote on Tue, 17 Aug 2021 13:36:03
> +0300:
>
> > Hi Miquel,
> >
> > On 16.08.2021 11:01, Miquel Raynal wrote:
> > > Hi Andy,
> > >
> > > Andy Shevchenko <[email protected]> wrote on Thu, 12 Aug 2021
> > > 15:13:10 +0300:
> > >
> > >> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]> wrote:
> > >>
> > >>> It seems that mxic_nfc_probe() missed invocation of
> > >>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error handling
> > >>> was refined appropriately.
> > >>
> > >> NAK. Until you provide a deeper analysis, like how this works before your
> > >> change.
> > >>
> > >>
> > >> Please, don’t blindly generate patches, this can even your bot do, just
> > >> think about each change and preferable test on the real hardware.
> > >>
> > >> The above is to all your lovely contributions.
> > >>
> > >>
> > >>> Found by Linux Driver Verification project (linuxtesting.org).
> > >>>
> > >>> Signed-off-by: Evgeny Novikov <[email protected]>
> > >>> Co-developed-by: Kirill Shilimanov <[email protected]>
> > >>> Signed-off-by: Kirill Shilimanov <[email protected]>
> > >>> ---
> > >>> drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> > >>> 1 file changed, 12 insertions(+), 4 deletions(-)
> > >>>
> > >>> diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_
> > >>> nand.c
> > >>> index da1070993994..37e75bf60ee5 100644
> > >>> --- a/drivers/mtd/nand/raw/mxic_nand.c
> > >>> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> > >>> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct platform_device
> > >>> *pdev)
> > >>> if (IS_ERR(nfc->send_dly_clk))
> > >>> return PTR_ERR(nfc->send_dly_clk);
> > >>>
> > >>> + err = mxic_nfc_clk_enable(nfc);
> > >>> + if (err)
> > >>> + return err;
> > > As Andy said, this is not needed.
> > >
> > >>> +
> > >>> nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > >>> - if (IS_ERR(nfc->regs))
> > >>> - return PTR_ERR(nfc->regs);
> > >>> + if (IS_ERR(nfc->regs)) {
> > >>> + err = PTR_ERR(nfc->regs);
> > >>> + goto fail;
> > >>> + }
> > >>>
> > >>> nand_chip = &nfc->chip;
> > >>> mtd = nand_to_mtd(nand_chip);
> > >>> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct platform_device
> > >>> *pdev)
> > >>> nand_chip->controller = &nfc->controller;
> > >>>
> > >>> irq = platform_get_irq(pdev, 0);
> > >>> - if (irq < 0)
> > >>> - return irq;
> > >>> + if (irq < 0) {
> > >>> + err = irq;
> > >>> + goto fail;
> > > However some reworking is needed in the error path.
> > >
> > > That goto statement should be renamed and devm_request_irq() should not
> > > jump to it.
> > >
> >
> > We still need some help and clarification from those who are very familiar with this sort of drivers or/and can test this particular driver. mxic_nfc_clk_enable() is the complementary function for mxic_nfc_clk_disable(). No other functions invoke clk_prepare_enable()/clk_disable_unprepare() in the driver. Unlikely somebody in its environment does that since driver specific clocks are dealt with. At the moment the driver invokes mxic_nfc_clk_disable() on error handling paths in probe, in remove and in mxic_nfc_set_freq(). mxic_nfc_clk_enable() is called just by mxic_nfc_set_freq() that moreover does this after calling mxic_nfc_clk_disable() first. So, we did not find any place in the driver that invokes mxic_nfc_clk_enable() prior to mxic_nfc_clk_disable(). Basing on this we added mxic_nfc_clk_enable() just after getting clocks. As I explained in the previous large e-mail, we may be wrong in our understanding of the driver environment or/and at specification of requirements being checked. It would be great if you will point out on our mistakes.
>
> Enabling the clocks seems to only be needed to access the NAND device
> and not the registers of the controller. Mason, is this statement
> right? If this statement is wrong, then your proposal is not entirely
> wrong in the sense that we must enable the missing clock *before*
> accessing any register.
>
> Otherwise for the two other clocks, we don't really care to enable them
> before setting the actual frequency in ->setup_interface() (called by
> nand_scan()) because at this point we don't yet know what timing mode
> is best. Disabling the clock is not an issue even though it was not
> enabled in the fist place anyway.
>
> In all cases, the error path is wrong.
>
> Thanks,
> Miquèl
On Tue, Aug 17, 2021 at 12:08 PM Evgeny Novikov <[email protected]> wrote:
> On 12.08.2021 15:13, Andy Shevchenko wrote:
> > On Thursday, August 12, 2021, Evgeny Novikov <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > It seems that mxic_nfc_probe() missed invocation of
> > mxic_nfc_clk_enable(). The patch fixed that. In addition, error
> > handling
> > was refined appropriately.
> >
> > NAK. Until you provide a deeper analysis, like how this works before
> > your change.
> >
> > Please, don’t blindly generate patches, this can even your bot do,
> > just think about each change and preferable test on the real hardware.
> >
> > The above is to all your lovely contributions.
>
> I completely agree with you that testing and debugging on the real
> hardware is indispensable since this can help to reason about both found
> bugs and patches suggested. Nevertheless, there are several cases when
> this does not work. The most obvious one is lack of appropriate devices
> on the spot that is not an obstacle for static analysis.
>
> My patches are based on results of static verification (software model
> checking). In a nutshell, this approach allows analyzing target programs
> more accurately in comparison with widely used static analysis tools.
> But this is not for free. Usually it needs much more computational
> resources to get something meaningful (in a general case the task is
> undecidable). Modern computer systems solve this issue partially. Worse
> is that thorough static analysis needs more or less complete and correct
> models of environments for target programs. For instance, for loadable
> kernel modules it is necessary to specify correct sequences of callback
> invocations, initialize their arguments at least to some extent and
> develop models of some vital functions like kzalloc(). Moreover, it is
> necessary to specify requirements if one wants to search for something
> special rather than NULL pointer dereferences. We were able to devote a
> relatively small part of our project to development of environment
> models and requirement specifications for verification of the Linux
> kernel. Also, we spent not so much time for application of our framework
> for open source device drivers. Nevertheless, this helped to find out
> quite a lot of bugs many of which are tricky enough. If you are
> interested in more details I can send you our last paper and presentation.
It is good and thanks for your contribution!
> Most our bug reports were accepted by developers. Rarely they preferred
> to fix bugs according to their needs and vision, that is especially the
> case for considerable changes that do need appropriate hardware and
> testing. Just a few our bug reports were ignored or rejected.
This ratio is not a point. I hope you learnt from the UMN case.
> In the
> latter case developers often pointed out us what is wrong with our
> current understanding and models of the device driver environment or
> requirement specifications. We always welcome this feedback as it allows
> us to refine the stuff and, thus, to avoid false alarms and invalid
> patches. Some developers requested scripts used for finding reported
> issues, but it is not easy to add or refer them in patches like, say,
> for Coccinelle since there is a bunch of external files developed in
> different domain-specific languages as well as a huge verification
> framework, that nobody will include into the source tree of the Linux
> kernel.
>
> Regarding your claim. We do not have an appropriate hardware. As usual
> this bug report was prepared on the base of static verification results
> purely. If you want to see on a particular artifact I based my decision
> on, I can share a link to a visualized violation witness provided by a
> verification tool. We have not any bots since used verification tools do
> not give any suggestions on the issue roots. Maybe in some cases it is
> possible to generate patches automatically on the base of these
> verification results like, say, Coccinelle does, but it is another big
> work. Of course, it is necessary to test the driver and confirm that
> there is an issue or reject the patch. As always the feedback is welcome.
My point is that the type of patches you are sending even a bot may
generate (for example, simple patches the LKP project generates along
with reports). The problem with all teams that are working with static
analysers against Linux kernel is that they so proud of their tools
and trying to flood the mailing lists with quick and nice fixes, from
which some are churn, some are simple bad, some are _bringing_
regressions, and only some are good enough. The ratio seems to me like
1 to 4 at most. So, 75% patches are not needed and only are a burden
on maintainers' shoulders.
Good patch should have a good commit message [1]. The message should
include an analysis to explain why the considered change is needed and
what the problem it tries to solve. Neither of this I have seen in
your patch. Also, you need to take into account the credits and tags
that Linux kernel is using (Fixes, Suggested-by, Reported-by, etc) it
will add a bit of unification. Also, while fixing problems these
patches often miss the big picture, and contributors should think
outside the box (this is a problem of 95% of such contributions, one
example is your patch where I recommended completely rewriting the
->probe() approach). That said, I don't want to see the flood of
patches with 75% miss ratio, I want to see maybe 5x, 10x less patches,
but each of them is carefully thought through and _ideally_ be tested
besides compilation.
And thank you for your work!
> If you are not gratified with my explanation it would be great if you
> and other developers will suggest any ideas how to enhance the process
> if you find this relevant.
[1]: https://chris.beams.io/posts/git-commit/
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 17.08.2021 14:47, Andy Shevchenko wrote:
> On Tue, Aug 17, 2021 at 12:08 PM Evgeny Novikov <[email protected]> wrote:
>> On 12.08.2021 15:13, Andy Shevchenko wrote:
>>> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]
>>> <mailto:[email protected]>> wrote:
>>>
>>> It seems that mxic_nfc_probe() missed invocation of
>>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error
>>> handling
>>> was refined appropriately.
>>>
>>> NAK. Until you provide a deeper analysis, like how this works before
>>> your change.
>>>
>>> Please, don’t blindly generate patches, this can even your bot do,
>>> just think about each change and preferable test on the real hardware.
>>>
>>> The above is to all your lovely contributions.
>> I completely agree with you that testing and debugging on the real
>> hardware is indispensable since this can help to reason about both found
>> bugs and patches suggested. Nevertheless, there are several cases when
>> this does not work. The most obvious one is lack of appropriate devices
>> on the spot that is not an obstacle for static analysis.
>>
>> My patches are based on results of static verification (software model
>> checking). In a nutshell, this approach allows analyzing target programs
>> more accurately in comparison with widely used static analysis tools.
>> But this is not for free. Usually it needs much more computational
>> resources to get something meaningful (in a general case the task is
>> undecidable). Modern computer systems solve this issue partially. Worse
>> is that thorough static analysis needs more or less complete and correct
>> models of environments for target programs. For instance, for loadable
>> kernel modules it is necessary to specify correct sequences of callback
>> invocations, initialize their arguments at least to some extent and
>> develop models of some vital functions like kzalloc(). Moreover, it is
>> necessary to specify requirements if one wants to search for something
>> special rather than NULL pointer dereferences. We were able to devote a
>> relatively small part of our project to development of environment
>> models and requirement specifications for verification of the Linux
>> kernel. Also, we spent not so much time for application of our framework
>> for open source device drivers. Nevertheless, this helped to find out
>> quite a lot of bugs many of which are tricky enough. If you are
>> interested in more details I can send you our last paper and presentation.
> It is good and thanks for your contribution!
>
>> Most our bug reports were accepted by developers. Rarely they preferred
>> to fix bugs according to their needs and vision, that is especially the
>> case for considerable changes that do need appropriate hardware and
>> testing. Just a few our bug reports were ignored or rejected.
> This ratio is not a point. I hope you learnt from the UMN case.
>
>> In the
>> latter case developers often pointed out us what is wrong with our
>> current understanding and models of the device driver environment or
>> requirement specifications. We always welcome this feedback as it allows
>> us to refine the stuff and, thus, to avoid false alarms and invalid
>> patches. Some developers requested scripts used for finding reported
>> issues, but it is not easy to add or refer them in patches like, say,
>> for Coccinelle since there is a bunch of external files developed in
>> different domain-specific languages as well as a huge verification
>> framework, that nobody will include into the source tree of the Linux
>> kernel.
>>
>> Regarding your claim. We do not have an appropriate hardware. As usual
>> this bug report was prepared on the base of static verification results
>> purely. If you want to see on a particular artifact I based my decision
>> on, I can share a link to a visualized violation witness provided by a
>> verification tool. We have not any bots since used verification tools do
>> not give any suggestions on the issue roots. Maybe in some cases it is
>> possible to generate patches automatically on the base of these
>> verification results like, say, Coccinelle does, but it is another big
>> work. Of course, it is necessary to test the driver and confirm that
>> there is an issue or reject the patch. As always the feedback is welcome.
> My point is that the type of patches you are sending even a bot may
> generate (for example, simple patches the LKP project generates along
> with reports). The problem with all teams that are working with static
> analysers against Linux kernel is that they so proud of their tools
> and trying to flood the mailing lists with quick and nice fixes, from
> which some are churn, some are simple bad, some are _bringing_
> regressions, and only some are good enough. The ratio seems to me like
> 1 to 4 at most. So, 75% patches are not needed and only are a burden
> on maintainers' shoulders.
Developers of static analysis tools need some acknowledgment.
Application to the Linux kernel gives a great capability for that since
it is a huge and vital open source project. Besides, it is unlikely that
somebody will be able to develop any valuable QA tool without a numerous
feedback from users (in case of this sort of tools users are developers
of target projects). We always welcome any ideas and suggestions how to
improve a quality of analysis.
> Good patch should have a good commit message [1]. The message should
> include an analysis to explain why the considered change is needed and
> what the problem it tries to solve. Neither of this I have seen in
> your patch. Also, you need to take into account the credits and tags
> that Linux kernel is using (Fixes, Suggested-by, Reported-by, etc) it
> will add a bit of unification. Also, while fixing problems these
> patches often miss the big picture, and contributors should think
> outside the box (this is a problem of 95% of such contributions, one
> example is your patch where I recommended completely rewriting the
> ->probe() approach). That said, I don't want to see the flood of
> patches with 75% miss ratio, I want to see maybe 5x, 10x less patches,
> but each of them is carefully thought through and _ideally_ be tested
> besides compilation.
We will try to follow your advices to a possible extent. I am not sure
that this will be the case for thinking outside the box since often it
requires a deep involvement into the development process. Moreover, it
may be dangerous to make such big changes without having an appropriate
experience or/and an ability to test them.
> And thank you for your work!
Thank you for your patience!
Best regards,
Evgeny Novikov
>> If you are not gratified with my explanation it would be great if you
>> and other developers will suggest any ideas how to enhance the process
>> if you find this relevant.
> [1]: https://chris.beams.io/posts/git-commit/
>
On Sat, Aug 21, 2021 at 8:26 PM Evgeny Novikov <[email protected]> wrote:
> On 17.08.2021 14:47, Andy Shevchenko wrote:
> > On Tue, Aug 17, 2021 at 12:08 PM Evgeny Novikov <[email protected]> wrote:
> >> On 12.08.2021 15:13, Andy Shevchenko wrote:
> >>> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]
> >>> <mailto:[email protected]>> wrote:
> >>>
> >>> It seems that mxic_nfc_probe() missed invocation of
> >>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error
> >>> handling
> >>> was refined appropriately.
> >>>
> >>> NAK. Until you provide a deeper analysis, like how this works before
> >>> your change.
> >>>
> >>> Please, don’t blindly generate patches, this can even your bot do,
> >>> just think about each change and preferable test on the real hardware.
> >>>
> >>> The above is to all your lovely contributions.
> >> I completely agree with you that testing and debugging on the real
> >> hardware is indispensable since this can help to reason about both found
> >> bugs and patches suggested. Nevertheless, there are several cases when
> >> this does not work. The most obvious one is lack of appropriate devices
> >> on the spot that is not an obstacle for static analysis.
> >>
> >> My patches are based on results of static verification (software model
> >> checking). In a nutshell, this approach allows analyzing target programs
> >> more accurately in comparison with widely used static analysis tools.
> >> But this is not for free. Usually it needs much more computational
> >> resources to get something meaningful (in a general case the task is
> >> undecidable). Modern computer systems solve this issue partially. Worse
> >> is that thorough static analysis needs more or less complete and correct
> >> models of environments for target programs. For instance, for loadable
> >> kernel modules it is necessary to specify correct sequences of callback
> >> invocations, initialize their arguments at least to some extent and
> >> develop models of some vital functions like kzalloc(). Moreover, it is
> >> necessary to specify requirements if one wants to search for something
> >> special rather than NULL pointer dereferences. We were able to devote a
> >> relatively small part of our project to development of environment
> >> models and requirement specifications for verification of the Linux
> >> kernel. Also, we spent not so much time for application of our framework
> >> for open source device drivers. Nevertheless, this helped to find out
> >> quite a lot of bugs many of which are tricky enough. If you are
> >> interested in more details I can send you our last paper and presentation.
> > It is good and thanks for your contribution!
> >
> >> Most our bug reports were accepted by developers. Rarely they preferred
> >> to fix bugs according to their needs and vision, that is especially the
> >> case for considerable changes that do need appropriate hardware and
> >> testing. Just a few our bug reports were ignored or rejected.
> > This ratio is not a point. I hope you learnt from the UMN case.
> >
> >> In the
> >> latter case developers often pointed out us what is wrong with our
> >> current understanding and models of the device driver environment or
> >> requirement specifications. We always welcome this feedback as it allows
> >> us to refine the stuff and, thus, to avoid false alarms and invalid
> >> patches. Some developers requested scripts used for finding reported
> >> issues, but it is not easy to add or refer them in patches like, say,
> >> for Coccinelle since there is a bunch of external files developed in
> >> different domain-specific languages as well as a huge verification
> >> framework, that nobody will include into the source tree of the Linux
> >> kernel.
> >>
> >> Regarding your claim. We do not have an appropriate hardware. As usual
> >> this bug report was prepared on the base of static verification results
> >> purely. If you want to see on a particular artifact I based my decision
> >> on, I can share a link to a visualized violation witness provided by a
> >> verification tool. We have not any bots since used verification tools do
> >> not give any suggestions on the issue roots. Maybe in some cases it is
> >> possible to generate patches automatically on the base of these
> >> verification results like, say, Coccinelle does, but it is another big
> >> work. Of course, it is necessary to test the driver and confirm that
> >> there is an issue or reject the patch. As always the feedback is welcome.
> > My point is that the type of patches you are sending even a bot may
> > generate (for example, simple patches the LKP project generates along
> > with reports). The problem with all teams that are working with static
> > analysers against Linux kernel is that they so proud of their tools
> > and trying to flood the mailing lists with quick and nice fixes, from
> > which some are churn, some are simple bad, some are _bringing_
> > regressions, and only some are good enough. The ratio seems to me like
> > 1 to 4 at most. So, 75% patches are not needed and only are a burden
> > on maintainers' shoulders.
> Developers of static analysis tools need some acknowledgment.
> Application to the Linux kernel gives a great capability for that since
> it is a huge and vital open source project. Besides, it is unlikely that
> somebody will be able to develop any valuable QA tool without a numerous
> feedback from users (in case of this sort of tools users are developers
> of target projects). We always welcome any ideas and suggestions how to
> improve a quality of analysis.
Good luck with it!
> > Good patch should have a good commit message [1]. The message should
> > include an analysis to explain why the considered change is needed and
> > what the problem it tries to solve. Neither of this I have seen in
> > your patch. Also, you need to take into account the credits and tags
> > that Linux kernel is using (Fixes, Suggested-by, Reported-by, etc) it
> > will add a bit of unification. Also, while fixing problems these
> > patches often miss the big picture, and contributors should think
> > outside the box (this is a problem of 95% of such contributions, one
> > example is your patch where I recommended completely rewriting the
> > ->probe() approach). That said, I don't want to see the flood of
> > patches with 75% miss ratio, I want to see maybe 5x, 10x less patches,
> > but each of them is carefully thought through and _ideally_ be tested
> > besides compilation.
> We will try to follow your advices to a possible extent. I am not sure
> that this will be the case for thinking outside the box since often it
> requires a deep involvement into the development process.
Exactly my point. You need to dive into development better.
> Moreover, it
> may be dangerous to make such big changes without having an appropriate
> experience or/and an ability to test them.
> > And thank you for your work!
> Thank you for your patience!
> >> If you are not gratified with my explanation it would be great if you
> >> and other developers will suggest any ideas how to enhance the process
> >> if you find this relevant.
> > [1]: https://chris.beams.io/posts/git-commit/
--
With Best Regards,
Andy Shevchenko
Hi Miquel,
+ Jaime from Macronix.
Jaime will check and confirm this issue.
> Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe
>
>
> Miquel Raynal <[email protected]> wrote on Tue, 17 Aug 2021
> 12:55:21 +0200:
>
> > Hi Evgeny,
> >
> > +Mason from Macronix
>
> Mason's e-mail bounced, including YouChing in the discussion who may
> also have an answer or perhaps knows who to include in the discussion
> (see below for the context and question).
>
> >
> > Evgeny Novikov <[email protected]> wrote on Tue, 17 Aug 2021 13:36:03
> > +0300:
> >
> > > Hi Miquel,
> > >
> > > On 16.08.2021 11:01, Miquel Raynal wrote:
> > > > Hi Andy,
> > > >
> > > > Andy Shevchenko <[email protected]> wrote on Thu, 12 Aug
2021
> > > > 15:13:10 +0300:
> > > >
> > > >> On Thursday, August 12, 2021, Evgeny Novikov <[email protected]>
wrote:
> > > >>
> > > >>> It seems that mxic_nfc_probe() missed invocation of
> > > >>> mxic_nfc_clk_enable(). The patch fixed that. In addition, error
handling
> > > >>> was refined appropriately.
> > > >>
> > > >> NAK. Until you provide a deeper analysis, like how this works
before your
> > > >> change.
> > > >>
> > > >>
> > > >> Please, don’t blindly generate patches, this can even your bot
do, just
> > > >> think about each change and preferable test on the real hardware.
> > > >>
> > > >> The above is to all your lovely contributions.
> > > >>
> > > >>
> > > >>> Found by Linux Driver Verification project (linuxtesting.org).
> > > >>>
> > > >>> Signed-off-by: Evgeny Novikov <[email protected]>
> > > >>> Co-developed-by: Kirill Shilimanov
<[email protected]>
> > > >>> Signed-off-by: Kirill Shilimanov <[email protected]>
> > > >>> ---
> > > >>> drivers/mtd/nand/raw/mxic_nand.c | 16 ++++++++++++----
> > > >>> 1 file changed, 12 insertions(+), 4 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/mtd/nand/raw/mxic_nand.c
b/drivers/mtd/nand/raw/mxic_
> > > >>> nand.c
> > > >>> index da1070993994..37e75bf60ee5 100644
> > > >>> --- a/drivers/mtd/nand/raw/mxic_nand.c
> > > >>> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> > > >>> @@ -509,9 +509,15 @@ static int mxic_nfc_probe(struct
platform_device
> > > >>> *pdev)
> > > >>> if (IS_ERR(nfc->send_dly_clk))
> > > >>> return PTR_ERR(nfc->send_dly_clk);
> > > >>>
> > > >>> + err = mxic_nfc_clk_enable(nfc);
> > > >>> + if (err)
> > > >>> + return err;
> > > > As Andy said, this is not needed.
> > > >
> > > >>> +
> > > >>> nfc->regs = devm_platform_ioremap_resource(pdev, 0);
> > > >>> - if (IS_ERR(nfc->regs))
> > > >>> - return PTR_ERR(nfc->regs);
> > > >>> + if (IS_ERR(nfc->regs)) {
> > > >>> + err = PTR_ERR(nfc->regs);
> > > >>> + goto fail;
> > > >>> + }
> > > >>>
> > > >>> nand_chip = &nfc->chip;
> > > >>> mtd = nand_to_mtd(nand_chip);
> > > >>> @@ -527,8 +533,10 @@ static int mxic_nfc_probe(struct
platform_device
> > > >>> *pdev)
> > > >>> nand_chip->controller = &nfc->controller;
> > > >>>
> > > >>> irq = platform_get_irq(pdev, 0);
> > > >>> - if (irq < 0)
> > > >>> - return irq;
> > > >>> + if (irq < 0) {
> > > >>> + err = irq;
> > > >>> + goto fail;
> > > > However some reworking is needed in the error path.
> > > >
> > > > That goto statement should be renamed and devm_request_irq()
should not
> > > > jump to it.
> > > >
> > >
> > > We still need some help and clarification from those who are very
familiar
> with this sort of drivers or/and can test this particular driver.
> mxic_nfc_clk_enable() is the complementary function for
mxic_nfc_clk_disable
> (). No other functions invoke
clk_prepare_enable()/clk_disable_unprepare() in
> the driver. Unlikely somebody in its environment does that since driver
> specific clocks are dealt with. At the moment the driver invokes
> mxic_nfc_clk_disable() on error handling paths in probe, in remove and
in
> mxic_nfc_set_freq(). mxic_nfc_clk_enable() is called just by
mxic_nfc_set_freq
> () that moreover does this after calling mxic_nfc_clk_disable() first.
So, we
> did not find any place in the driver that invokes mxic_nfc_clk_enable()
prior
> to mxic_nfc_clk_disable(). Basing on this we added mxic_nfc_clk_enable()
just
> after getting clocks. As I explained in the previous large e-mail, we
may be
> wrong in our understanding of the driver environment or/and at
specification
> of requirements being checked. It would be great if you will point out
on our mistakes.
> >
> > Enabling the clocks seems to only be needed to access the NAND device
> > and not the registers of the controller. Mason, is this statement
> > right? If this statement is wrong, then your proposal is not entirely
> > wrong in the sense that we must enable the missing clock *before*
> > accessing any register.
> >
> > Otherwise for the two other clocks, we don't really care to enable
them
> > before setting the actual frequency in ->setup_interface() (called by
> > nand_scan()) because at this point we don't yet know what timing mode
> > is best. Disabling the clock is not an issue even though it was not
> > enabled in the fist place anyway.
> >
> > In all cases, the error path is wrong.
> >
> > Thanks,
> > Miquèl
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.
Macronix International Co., Ltd.
=====================================================================
============================================================================
CONFIDENTIALITY NOTE:
This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.
Macronix International Co., Ltd.
=====================================================================