| Message ID | 1573541519-28488-5-git-send-email-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/13] media: uvcvideo: drop error check of debugfs_create_dir() | expand |
Hi, On Tue, Nov 12, 2019 at 02:51:51PM +0800, Chunfeng Yun wrote: > Now the USB gadget subsystem can use the USB debugfs root directory, > so move musb's directory from the root of the debugfs filesystem into > the root of usb My opinion is this move is unnecessary. I breaks existing debug tools or documentation which is already published on Internet. -Bin.
On Tue, Nov 12, 2019 at 09:28:57AM -0600, Bin Liu wrote: > Hi, > > On Tue, Nov 12, 2019 at 02:51:51PM +0800, Chunfeng Yun wrote: > > Now the USB gadget subsystem can use the USB debugfs root directory, > > so move musb's directory from the root of the debugfs filesystem into > > the root of usb > > My opinion is this move is unnecessary. I breaks existing debug tools or > documentation which is already published on Internet. Having a "root" directory for a single random driver seems like you are making your driver a "very important" thing in the overall scheme of the kernel, right? What's wrong with using the usb subdirectory like all other USB drivers use (after this patch series is merged)? That feels like a much more "sane" way to handle the wide-open debugfs namespace. Yes, there are no rules when it comes to debugfs file names and locations, but let's try to be sane please. thanks, greg k-h
On Thu, Nov 14, 2019 at 11:20:18AM +0800, Greg Kroah-Hartman wrote: > On Tue, Nov 12, 2019 at 09:28:57AM -0600, Bin Liu wrote: > > Hi, > > > > On Tue, Nov 12, 2019 at 02:51:51PM +0800, Chunfeng Yun wrote: > > > Now the USB gadget subsystem can use the USB debugfs root directory, > > > so move musb's directory from the root of the debugfs filesystem into > > > the root of usb > > > > My opinion is this move is unnecessary. I breaks existing debug tools or > > documentation which is already published on Internet. > > Having a "root" directory for a single random driver seems like you are > making your driver a "very important" thing in the overall scheme of the > kernel, right? What's wrong with using the usb subdirectory like all Agree, it wasn't the right thing to do at the first place. But now changing it adds support burden, because people very often refer to the old information on the internet which no longer matches to the new location. Basically, it is a cost of ABI change. > other USB drivers use (after this patch series is merged)? That feels > like a much more "sane" way to handle the wide-open debugfs namespace. Though I commented on this musb patch, my opinion is for this whole series, either drop the whole series or apply the whole series. > Yes, there are no rules when it comes to debugfs file names and > locations, but let's try to be sane please. Fine with me. I can still support questions such as "can't open /sys/kernel/debug/musb-hdrc.0/testmode: No such file or directory". -Bin.
On Thu, Nov 14, 2019 at 08:02:34AM -0600, Bin Liu wrote: > On Thu, Nov 14, 2019 at 11:20:18AM +0800, Greg Kroah-Hartman wrote: > > On Tue, Nov 12, 2019 at 09:28:57AM -0600, Bin Liu wrote: > > > Hi, > > > > > > On Tue, Nov 12, 2019 at 02:51:51PM +0800, Chunfeng Yun wrote: > > > > Now the USB gadget subsystem can use the USB debugfs root directory, > > > > so move musb's directory from the root of the debugfs filesystem into > > > > the root of usb > > > > > > My opinion is this move is unnecessary. I breaks existing debug tools or > > > documentation which is already published on Internet. > > > > Having a "root" directory for a single random driver seems like you are > > making your driver a "very important" thing in the overall scheme of the > > kernel, right? What's wrong with using the usb subdirectory like all > > Agree, it wasn't the right thing to do at the first place. But now > changing it adds support burden, because people very often refer to the > old information on the internet which no longer matches to the new > location. Basically, it is a cost of ABI change. What information says that /sys/kernel/debug/mdev/ is the location for this? Is it in-kernel? > > other USB drivers use (after this patch series is merged)? That feels > > like a much more "sane" way to handle the wide-open debugfs namespace. > > Though I commented on this musb patch, my opinion is for this whole > series, either drop the whole series or apply the whole series. I've applied all but this one and 2 others that did not build properly. > > Yes, there are no rules when it comes to debugfs file names and > > locations, but let's try to be sane please. > > Fine with me. I can still support questions such as "can't open > /sys/kernel/debug/musb-hdrc.0/testmode: No such file or directory". What tool looks for that? thanks, greg k-h
On Fri, Nov 15, 2019 at 11:21:53AM +0800, Greg Kroah-Hartman wrote: > On Thu, Nov 14, 2019 at 08:02:34AM -0600, Bin Liu wrote: > > On Thu, Nov 14, 2019 at 11:20:18AM +0800, Greg Kroah-Hartman wrote: > > > On Tue, Nov 12, 2019 at 09:28:57AM -0600, Bin Liu wrote: > > > > Hi, > > > > > > > > On Tue, Nov 12, 2019 at 02:51:51PM +0800, Chunfeng Yun wrote: > > > > > Now the USB gadget subsystem can use the USB debugfs root directory, > > > > > so move musb's directory from the root of the debugfs filesystem into > > > > > the root of usb > > > > > > > > My opinion is this move is unnecessary. I breaks existing debug tools or > > > > documentation which is already published on Internet. > > > > > > Having a "root" directory for a single random driver seems like you are > > > making your driver a "very important" thing in the overall scheme of the > > > kernel, right? What's wrong with using the usb subdirectory like all > > > > Agree, it wasn't the right thing to do at the first place. But now > > changing it adds support burden, because people very often refer to the > > old information on the internet which no longer matches to the new > > location. Basically, it is a cost of ABI change. > > What information says that /sys/kernel/debug/mdev/ is the location for > this? Is it in-kernel? No, they are not in-kernel, but many places mainly on https://e2e.ti.com and http://processors.wiki.ti.com. It basically says to get musb regdump, cat /sys/kernel/debug/musb-hdrc.{0,1}/regdump, or to enter test mode, do echo _testmode_ > /sys/kernel/debug/musb-hdrc.{0,1}/testmode... > > > other USB drivers use (after this patch series is merged)? That feels > > > like a much more "sane" way to handle the wide-open debugfs namespace. > > > > Though I commented on this musb patch, my opinion is for this whole > > series, either drop the whole series or apply the whole series. > > I've applied all but this one and 2 others that did not build properly. Okay. > > > Yes, there are no rules when it comes to debugfs file names and > > > locations, but let's try to be sane please. > > > > Fine with me. I can still support questions such as "can't open > > /sys/kernel/debug/musb-hdrc.0/testmode: No such file or directory". > > What tool looks for that? I wrote a usb diagnosis tool called chkusb.sh, which basically checks usb related information in - /proc/config.gz - /lib/modules/`uname -r`/kernel/drivers/ - /proc/device-tree/ - /sys/device/ - /sys/kernel/debug/ So whenever I got a report on e2e.ti.com saying that usb doesn't work, I just post this script and get a log which would give a clue if there was any fundamental mistake in kernel configuration or deployment. But not a big issue here for this tool regarding this patch set, I can update the script to check both locations - /sys/kernel/debug/ and /sys/kernel/debug/usb/. -Bin.
diff --git a/drivers/usb/musb/musb_debugfs.c b/drivers/usb/musb/musb_debugfs.c index f42858e2b54c..7b6281ab62ed 100644 --- a/drivers/usb/musb/musb_debugfs.c +++ b/drivers/usb/musb/musb_debugfs.c @@ -325,7 +325,7 @@ void musb_init_debugfs(struct musb *musb) { struct dentry *root; - root = debugfs_create_dir(dev_name(musb->controller), NULL); + root = debugfs_create_dir(dev_name(musb->controller), usb_debug_root); musb->debugfs_root = root; debugfs_create_file("regdump", S_IRUGO, root, musb, &musb_regdump_fops);
Now the USB gadget subsystem can use the USB debugfs root directory, so move musb's directory from the root of the debugfs filesystem into the root of usb Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- v2: 1. abandon new API usb_debugfs_create_dir(), and use usb_debug_root --- drivers/usb/musb/musb_debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)