> -----Original Message----- > From: Randy Dunlap [mailto:rdunlap@xxxxxxxxxxxxx] > Sent: Sunday, November 8, 2020 5:12 PM > To: John Hubbard <jhubbard@xxxxxxxxxx>; Song Bao Hua (Barry Song) > <song.bao.hua@xxxxxxxxxxxxx>; akpm@xxxxxxxxxxxxxxxxxxxx; > linux-mm@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Cc: Linuxarm <linuxarm@xxxxxxxxxx>; Ralph Campbell > <rcampbell@xxxxxxxxxx>; John Garry <john.garry@xxxxxxxxxx> > Subject: Re: [PATCH] mm/gup_benchmark: GUP_BENCHMARK depends on > DEBUG_FS > > On 11/7/20 7:22 PM, John Hubbard wrote: > > On 11/7/20 7:14 PM, John Hubbard wrote: > >> On 11/7/20 6:58 PM, Song Bao Hua (Barry Song) wrote: > >>>> On 11/7/20 2:20 PM, Randy Dunlap wrote: > >>>>> On 11/7/20 11:16 AM, John Hubbard wrote: > >>>>>> On 11/7/20 11:05 AM, Song Bao Hua (Barry Song) wrote: > >>>>>>>> From: John Hubbard [mailto:jhubbard@xxxxxxxxxx] > >>>>>> ... > >>>> But if you really disagree, then I'd go with, just drop the patch entirely, > because > >>>> it doesn't really make things better as written...IMHO anyway. :) > >>> > >>> Just imagine a case, we don't enable DEBUG_FS but we enable GUP_TEST, > we will > >>> get an image with totally useless code section since GUP_TEST depends on > debugfs > >>> entry to perform any useful functionality. > >>> > >> > >> Looking at the choices, from the user's (usually kernel developer's) > experience: > >> > >> a) The user enables GUP_TEST, then boots up, runs, and is briefly surprised > by a > >> runtime failure. But it's a very quick diagnosis: "open: No such file or > directory", > >> when trying to make that ioctl call. The path indicates that it's a debug fs > path, > >> so the solution is pretty clear, at least for the main audience. > >> > >> b) The other choice: the user *never even sees* GUP_TEST as a choice. This > especially > >> bothers me because sometimes you find things by poking around in the > menu, although > >> of course "you should already know about it"...but there's a lot to "already > know" > >> in a large kernel. > >> > >> From a user experience, it's way better to simply see what you want, and > select it > >> in the menu. Or, at least get some prompt that you need to pre-select > something else. > >> > > > > ...and again, this is all fallout from Kconfig. I might be missing some advanced > > feature, because it seems surprising to only be allowed to choose between > > missing dependencies (which this patch would correct), or a poorer user > experience > > (which I argue that this patch would also provide). > > > > Ideally, we'd just set up the dependencies, and then have some options for > > visibility, but I'm not aware of any way to do that...and after a quick peek > > at Documentation/kbuild/kconfig-macro-language.rst it looks pretty bare > bones. > > Look at kconfig-language.rst instead. > > One thing that could be done (and is done in a few places for other reasons) is > to add > a Kconfig comment if DEBUG_FS is not enabled: > > comment "GUP_TEST needs to have DEBUG_FS enabled" > depends on !GUP_TEST && !DEBUG_FS > > e.g. drivers/hid/usbhid/Kconfig: > > comment "Input core support is needed for USB HID input layer or HIDBP > support" > depends on USB_HID && INPUT=n This is interesting. Thanks for sharing this. I've never realized we can do this kind of comments. What I have been always doing is simply reading the status from menuconfig. For example, to use IIO_CONSUMERS_PER_TRIGGER, I search it in menuconfig by "/", I get: Symbol: IIO_CONSUMERS_PER_TRIGGER [=] │ Type : integer │ Defined at drivers/iio/Kconfig:41 │ Prompt: Maximum number of consumers per trigger │ Depends on: IIO [=n] && IIO_TRIGGER [=n] │ Location: │ -> Device Drivers │ (1) -> Industrial I/O support (IIO [=n]) │ -> Enable triggered sampling support (IIO_TRIGGER [=n]) │ The menuconfig tells me it depends on IIO and IIO_TRIGGER. But both of them are "n". so the next step, I am going to make IIO and IIO_TRIGGER "y". > > > -- > ~Randy Thanks Barry
