Re: [PATCH 01/12] usb: xhci: expose xhci extended capabilities via debugfs

["Lu, Baolu" <baolu.lu@linux.intel.com>]

On 10/30/2015 10:45 PM, Greg Kroah-Hartman wrote:
> On Fri, Oct 30, 2015 at 08:09:17PM +0800, Lu, Baolu wrote:
>>
>> On 10/28/2015 08:40 PM, Greg Kroah-Hartman wrote:
>>>> +static const char *get_extcap_desc(u32 cap_id)
>>>>> +{
>>>>> +	switch (cap_id) {
>>>>> +	case XHCI_EXT_CAPS_LEGACY:
>>>>> +		return "USB Legacy Support";
>>>>> +	case XHCI_EXT_CAPS_PROTOCOL:
>>>>> +		return "Supported Protocol";
>>>>> +	case XHCI_EXT_CAPS_PM:
>>>>> +		return "Extended Power Management";
>>>>> +	case XHCI_EXT_CAPS_VIRT:
>>>>> +		return "I/O Virtualization (xHCI-IOV)";
>>>>> +	case XHCI_EXT_CAPS_ROUTE:
>>>>> +		return "Message Interrupt";
>>>>> +	case XHCI_EXT_CAPS_LOCALMEM:
>>>>> +		return "Local Memory";
>>>>> +	case XHCI_EXT_CAPS_DEBUG:
>>>>> +		return "USB Debug Capability";
>>> This is a lot more stuff than just debug port, it should be in sysfs
>>> as individual files, not one big one that you somehow have to parse in
>>> order to determine this information.
>>>
>> Hi Greg,
>>
>> It's hard to put each extended capability into a individual sysfs file.
> Agreed.
>
>> The extended capabilities are optional. One extended capability
>> might be supported in one hardware, but not in another. Also,
>> there are many "vendor defined" capabilities (ID range 192-255).
>> The vendor defined capabilities are not defined in xhci spec and
>> they could be used by the hardware vendor for various purposes.
>>
>> The purpose of this patch is to let user know what kind of extended
>> capabilities does a host controller supported. For example, on
>> one of my develop machines, it prints,
>>
>> @addr(virt)        CAP_ID    Description
>> @ffffc90001c88000    02    Supported Protocol
>> @ffffc90001c88020    02    Supported Protocol
>> @ffffc90001c88070    c0    Vendor Defined
>> @ffffc90001c8846c    01    USB Legacy Support
>> @ffffc90001c884f4    c6    Vendor Defined
>> @ffffc90001c88500    c7    Vendor Defined
>> @ffffc90001c88600    c2    Vendor Defined
>> @ffffc90001c88700    0a    USB Debug Capability
>> @ffffc90001c88740    c3    Vendor Defined
>> @ffffc90001c88800    c4    Vendor Defined
>> @ffffc90001c88900    c5    Vendor Defined
>>
>> With this output I know that "USB Debug Capability" is supported
>> in my machine.
> First off, why are you printing the address out?  Userspace never needs
> to see that.  Why not just iterate through the protocols and export the
> information as different files:
> 	protocol_XX
> and if the file is present or not describes if the hardware supports it
> or not.
>
> The issue with debugfs is that it is not enabled on all systems, and
> only can be read by the root user, so it is hard for people to find this
> information out if they want to do normal things with the hardware.

I agree with you that we should do this thru sysfs, not debugfs.

>
> But, if this really is only a debug thing, then it can say a debugfs
> file, but realize that almost no one will be able to see it (and even
> then, don't export the kernel addresses of the hardware.)

I realized that not all extended capabilities are valuable to users.

As far as I can see, debug capability (hw implements USB3 debug
port) is one that users should be interested at. Another one that
could be valuable is USB 3.1 speed protocol (root ports that support
USB 3.1 enhanced super speed).

For this time being, I will only implement sysfs node for debug
port. I will make a separated patch for USB 3.1 speed port after
I have Mathias' opinion.

>
> thanks,
>
> greg k-h

Thanks,
Baolu

> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>