Create New debugfs Entries

[ghost]

Create debugfs entries for visorchannel_debug() within visorbus. Each visorbus device should get a debugfs entry. Therefore it is recommended that they be added to the devices in visordriver_probe_device().

Follow @selltc's commit bcde487 for the general spirit of how to implement these entries.

[ghost]

Branch: visorbus_create_debugfs; commits: d95e57a381e64889e466a184764041004595ce25 Upstream: upstream-next checkpatch-cleared: yes T710-1 verified: yes

[selltc]

@TurningWrenches, I provided comments on your commit 178cb49547c59b4da32bd5caf6c86cc4cd01430b, to hopefully answer the questions from your email.

[ghost]

A couple questions:

1) In visorbus_init(), I wonder if I am missing something important by not logging the postcode found under the "error" label?

2) From what I've read, debugfs_create_dir() returns one of three outputs: a pointer on success, NULL if the parent is NULL, or -ENODEV if debugfs is not enabled in the kernel. I'm currently not checking for the -ENODEV case...should this be accounted for when calling this function, or do we assume our kernels have debugfs compiled-in? Another person says that if a pointer is returned, you can't guarantee success until you also verify IS_ERR() returns zero.

3) The same question as #2 applies for debugfs_create_file().

4) I don't include the queue code in the original visorchannel_debug() (see 6a98807c1bca074abec1da97727d88c0e21925e4) because I believe queue information is unavailable to us in this driver. Is this assumption correct?

[selltc]

1) - it might be useful to add another postcode, yes.

2) - you are doing the correct thing by NOT checking for -ENODEV. Your code SHOULD work ok if you compile withOUT DEBUGFS, but that case should be tested. Basically, you treat the -ENODEV result as a form of "success", and keep going. That's the way this is handled at other places in the kernel as well, and is actually the reason the kernel-wizards decided to have the special -ENODEV pointer result -- so the code "just works", regardless of whether DEBUGFS is defined. (Of course you won't have any debugfs entries if you compile without DEBUGFS, but the code will otherwise still function properly.)

3) - same answer as #2. Again, look at the xrefs and see how this is done other places in the kernel.

[selltc]

4) I believe you are already providing the queue information from commit 6a98807c1bca074abec1da97727d88c0e21925e4, in sigqueue_debug(). The visorchannel_signalqueue_slots_avail() and visorchannel_signalqueue_max_slots() removed in that patch were never used.

[ghost]

Thanks! I addressed your comments in 36fd7e16d3969cbfe031ee8cece36f588b9bded2.

[selltc]

This looks good to me, except for the visorchannel_debug() prototype you added to drivers/staging/unisys/visorbus/visorbus_private.h. What's that for?

[ghost]

Great catch, thanks. I was using 6a98807c1bca074abec1da97727d88c0e21925e4 as a template, and probably copied it over to visorbus_private.h during the early stages of writing this patch. In my patch, its no longer called visorchannel_debug(), but rather header_debugfs_show.

[ghost]

Fixed in d95e57a381e64889e466a184764041004595ce25.

[selltc]

Thanks; looks good.

[ghost]

To be thorough, I created a small kernel config by issuing a make tinyconfig, and then using make menuconfig to tweak just enough parameters to turn on the visorbus driver, but simultaneously exclude debugfs. I then used a modified version of our testing script to use this special kernel config file (and not replace it), and then performed a clean build (i.e. ran make clean first). The code compiled just fine.

[davidker]

Only issue I have is that Greg will want one item per debugfs entry so I don't believe this debugfsi valid.