netdev - RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <E607265CB020454880711A6F96C05A03893CD38E@hasmsx107.ger.corp.intel.com>
Date:	Thu, 14 Jul 2016 14:49:52 +0000
From:	"Levy, Amir (Jer)" <amir.jer.levy@...el.com>
To:	"Winkler, Tomas" <tomas.winkler@...el.com>,
	"andreas.noever@...il.com" <andreas.noever@...il.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"bhelgaas@...gle.com" <bhelgaas@...gle.com>
CC:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	thunderbolt-linux <thunderbolt-linux@...el.com>,
	"Westerberg, Mika" <mika.westerberg@...el.com>
Subject: RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM
 (firmware)

Hi Tomas,
Thanks for your comments.

On Thu, Jul 14 2016, 03:44 PM, Winkler, Tomas wrote:
> > +/* NHI genetlink commands */
> > +enum {
> > +	NHI_CMD_UNSPEC,
> > +	NHI_CMD_SUBSCRIBE,
> > +	NHI_CMD_UNSUBSCRIBE,
> > +	NHI_CMD_QUERY_INFORMATION,
> > +	NHI_CMD_MSG_TO_ICM,
> > +	NHI_CMD_MSG_FROM_ICM,
> > +	NHI_CMD_MAILBOX,
> > +	NHI_CMD_APPROVE_TBT_NETWORKING,
> > +	NHI_CMD_ICM_IN_SAFE_MODE,
> > +	__NHI_CMD_MAX,
> > +};
> > +#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
> NHI_CMD_MAX  = NHI_CMD_ICM_IN_SAFE_MODE ?
> 

This template is used a lot with (generic) netlink. 
Few examples:
http://lxr.free-electrons.com/source/drivers/acpi/event.c#L62
http://lxr.free-electrons.com/source/include/uapi/linux/irda.h#L224
It is easier to maintain - adding entry in the list will automatically update MAX.

[...]

> > +		u32 status;
> > +
> > +		status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
> > +
> > +		if (status & REG_FW_STS_NVM_AUTH_DONE)
> > +			break;
> > +		msleep(30);
> 
> 30 is  big number, for polling, what is behind this?
> 

The NVM authentication can take time for ICM.
This number comes from experiments.

[...]

> > +static struct tbt_nhi_ctxt *nhi_search_ctxt(u32 id)
> > +{
> > +	struct tbt_nhi_ctxt *nhi_ctxt;
> > +
> > +	list_for_each_entry(nhi_ctxt, &controllers_list, node)
> > +		if (nhi_ctxt->id == id)
> > +			return nhi_ctxt;
> 
> Don't you need to lock this list with the controllers_list_rwsem ?
> 

This is a helper function for searching the list.
The callers take and release the lock.

[...]

> > +	bool nvm_auth_on_boot : 1;
> Don't use bool with bit fields use u32 

What is the concern here?
If it is size of bool, it doesn't matter for this struct.
It is more readable that the expected value is bool,
and it is used a lot in kernel, for example:
http://lxr.free-electrons.com/source/include/linux/pm.h#L558