Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe

[Evgeny Novikov]

Hi Andy,

On 17.08.2021 14:47, Andy Shevchenko wrote:
> On Tue, Aug 17, 2021 at 12:08 PM Evgeny Novikov <novikov@xxxxxxxxx> wrote:
> > On 12.08.2021 15:13, Andy Shevchenko wrote:
> > > On Thursday, August 12, 2021, Evgeny Novikov <novikov@xxxxxxxxx
> > > <mailto:novikov@xxxxxxxxx>> 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/