Messages in this thread | | | Subject | Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices | From | Jeremy McNicoll <> | Date | Tue, 27 Feb 2018 14:27:26 -0800 |
| |
On 2018-02-27 1:26 AM, Mika Westerberg wrote: > On Mon, Feb 26, 2018 at 12:15:28PM -0800, Jeremy McNicoll wrote: >> On 2018-02-26 11:46 AM, Mika Westerberg wrote: >>> On Mon, Feb 26, 2018 at 11:28:16AM -0800, Jeremy McNicoll wrote: >>>> On 2018-02-26 5:38 AM, Mika Westerberg wrote: >>>>> On Mon, Feb 26, 2018 at 12:20:29PM +0200, Mika Westerberg wrote: >>>>>> On Thu, Feb 22, 2018 at 03:17:38PM -0800, Jeremy McNicoll wrote: >>>>>>>> + if (pkg->link_info & ICM_LINK_INFO_REJECTED) { >>>>>>>> + tb_info(tb, "switch at %u.%u was rejected by ICM firmware\n", >>>>>>>> + link, depth); >>>>>>> >>>>>>> This kind of condition sounds more like an error instead of info. >>>>>>> Please bump this up to tb_WARN/tb_warn ideally tb_err(). >>>>>> >>>>>> No, this is not an error. >>>>> >>>>> To be more clear, it is totally fine to have the firmware to reject some >>>>> devices. For example in case of the new usbonly security level the >>>>> firmware rejects other devices but the first. >>>>> >>>> >>>> Ok. Is that kind of information available to the kernel? What security >>>> mode we are in? >>>> >>>> ie) if (LINK_REJECTED && !USB_SECURITY) >>>> print "Error switch %u was rejected since its not usbonly" >>>> endif >>>> >>>> I am sure something like that simplified pseudo code above would >>>> be somewhat useful to users when debugging. >>> >>> That's why it is on info level so it goes to dmesg but does not scare >>> the user :-) >>> >> >> The point I am trying to make is that it would be nice to be able to >> know WHY the link was rejected and not just that it was rejected. > > Fair enough. In practice (since we ask the firmware to accept any > device) the only reason for rejection is that the topology limit is > exceeded (too many devices in the chain). > > I'm thinking to change the message to something like: > > tb_info(tb, "switch at %u.%u was rejected by ICM firmware because topology limit exceeded\n", > link, depth); > > And do the same for Titan Ridge in patch [18/18]. > > Security level can be read directly from "security" sysfs attribute of > the domain so that information does not need to be duplicated IMHO. > > Does that work for you? >
Sounds good to me.
-jeremy
| |