Re: [07/18] thunderbolt: Handle rejected Thunderbolt devices

[Mika Westerberg]

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?