Messages in this thread | | | From | Andy Shevchenko <> | Date | Sun, 22 Aug 2021 11:27:33 +0300 | Subject | Re: [PATCH] mtd: rawnand: mxic: Enable and prepare clocks in probe |
| |
On Sat, Aug 21, 2021 at 8:26 PM Evgeny Novikov <novikov@ispras.ru> wrote: > On 17.08.2021 14:47, Andy Shevchenko wrote: > > On Tue, Aug 17, 2021 at 12:08 PM Evgeny Novikov <novikov@ispras.ru> wrote: > >> On 12.08.2021 15:13, Andy Shevchenko wrote: > >>> On Thursday, August 12, 2021, Evgeny Novikov <novikov@ispras.ru > >>> <mailto:novikov@ispras.ru>> 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
| |