Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: z2jPtrzwLVDsDMtsc9YttuBuqUOw6GaSW7xb+67ZdW58M49aidc/9o8Sp4RYzAAhiQ9vN5NCx+
 JLiMFI+hjbrw==
IronPort-SDR: ObPJoH9a1Wlmw9Fuc5IrwBnMhSZ8lKANyOOyR4Ml+A1bG83bU0Ds+ke+5954WOmd2iPtMJGO1E
 HddFtwPI+YUw==
From:   "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "luca@coelho.fi" <luca@coelho.fi>,
        "Beker, Ayala" <ayala.beker@intel.com>,
        "Coelho, Luciano" <luciano.coelho@intel.com>
Subject: RE: [PATCH RESEND 2/3] iwlwifi: mei: add the driver to allow
 cooperation with CSME
Thread-Topic: [PATCH RESEND 2/3] iwlwifi: mei: add the driver to allow
 cooperation with CSME
Thread-Index: AQHXL5l8AAlmpHtWLk6IhaLef8ZD26qw2g6AgAAEIoCAABAHAIAAAg7g
Date:   Mon, 12 Apr 2021 14:29:45 +0000
Message-ID: <SA0PR11MB475228709B9B0B6D74CE07A2F2709@SA0PR11MB4752.namprd11.prod.outlook.com>
References: <20210412124328.24472-1-emmanuel.grumbach@intel.com>
 <20210412124328.24472-2-emmanuel.grumbach@intel.com>
 <YHRFy3aq/gB7Vde6@kroah.com>
 <SA0PR11MB475215678ADCCE657C6574D6F2709@SA0PR11MB4752.namprd11.prod.outlook.com>
 <YHRWtJqbuFGmR2Sw@kroah.com>
In-Reply-To: <YHRWtJqbuFGmR2Sw@kroah.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 39365d74-071d-4895-d135-08d8fdbf6b91
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Apr 2021 14:29:45.6890
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: DlvnIEr+N56odTSmGChka9jay30RXCIR6ewF1VB0hFuCHsHXBCSde1GKBBFw7nT3rWc6GMta2NYvEet5s2fspJ9QNuYVVL66Yx9U9ZAfxU0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4892
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

>=20
> On Mon, Apr 12, 2021 at 01:44:58PM +0000, Grumbach, Emmanuel wrote:
> > > > +#define IWL_MEI_DEBUG(c, f, a...)		\
> > > > +	do {					\
> > > > +		CHECK_FOR_NEWLINE(f);		\
> > >
> > > Huh?
> > >
> > > > +		dev_dbg(&(c)->dev, f, ## a);	\
> > >
> > > Just use dev_dbg(), don't be special for a single driver, it hurts
> > > when trying to read different drivers.
> >
> > I took this from iwlwifi. I can change if needed, not a big deal.
>=20
> Please do.
>=20
> > > > +module_param_named(defer_start_message, defer_start_message,
> > > bool,
> > > > +0644); MODULE_PARM_DESC(defer_start_message,
> > > > +		 "Defer the start message Tx to CSME (default false)");
> > >
> > > Why do you need this?  Who is going to set it to anything else, and
> > > why would they?  This isn't the 1990's anymore, please do not add
> > > new module parameters.
> >
> > For testing. I need this to be able to force a certain order of initial=
ization
> which is possible (and hence must be tested) but not likely to happen.
> > Another point is tracing. This allows me to load the module but prevent=
 any
> real operation. Then, start tracing. This way, I can see the whole flow i=
n
> tracing, even the very beginning.
>=20
> Then call this something obvious,
> "kernel_hacker_debuging_testing_only_use_if_you_know_what_you_are_
> doing".
>=20
> Or better yet, just put it in debugfs to turn it on/off and no module
> parameter is needed at all.
>=20

Debugfs is not a replacement for module parameters. Debugfs can be used onl=
y after the driver already ran quite a bit of its initialization code path.=
 Here I want to be able to catch the very first messages with tracing.

> > > > +	if (WARN_ON(rd > q_sz || wr > q_sz))
> > > > +		return -EINVAL;
> > >
> > > If any of the WARN_ON() things in this driver can ever trigger, just
> > > handle them normally and do NOT call WARN_ON() as you just rebooted
> > > the box for a simple thing that you could have recovered from.
> >
> > My understanding is that WARN_ON does not reboot the box which is why
> it was invented when we already had BUG_ON. This can't be triggered by th=
e
> user space, but can be triggered by the CSME firmware that runs on the
> chipset.
>=20
> when panic_on_warn is enabled, the box will reboot.
>=20
> If firmware can cause this, then properly handle the issue and continue o=
n,
> don't reboot machines.

I'll print an error then. I still didn't understand what's the difference b=
etween BUG_ON and WARN_ON in your eyes.

>=20
> > > Some of these WARN_ON() in the code feel very lazy as you control
> > > the caller so you "know" that this will never happen.  So don't
> > > leave them in, it's debugging code that you can now remove.
> >
> > I can transform them in error prints, but again, my understanding is / =
was
> that WARN_ON is ok to use and won't reboot the box since it differs from
> BUG_ON.
>=20
> Again, panic_on_warn.
>=20
> > > > +
> > > > +	IWL_MEI_DEBUG(cldev, "Got CSME filters\n");
> > >
> > > ftrace is your friend, remove these pointless "the code got here!"
> > > lines.  You have loads of them...
> >
> > This is debug. Won't appear unless you want it.
> > I have extensive tracing support.
>=20
> Again, use ftrace for tracing, don't use debug print messages.
>=20
> > > > +static void iwl_mei_handle_rx_host_own_req(struct mei_cl_device
> > > *cldev,
> > > > +					   const struct iwl_sap_msg_dw *dw)
> > > {
> > > > +	struct iwl_mei *mei =3D mei_cldev_get_drvdata(cldev);
> > > > +
> > > > +	IWL_MEI_DEBUG(cldev, "Got ownership req response: %d\n", dw-
> > > >val);
> > >
> > > Code got here!  :(
> >
> > Same, this is protected by a dynamic debug knob.
> > I can see tons of those in mei bus driver .
> > $ git grep _dbg\( -- drivers/misc/mei | wc -l
> > 228
>=20
> mei should be fixed as well.
>=20
> > > > +
> > > > +	if (!dw->val) {
> > > > +		IWL_MEI_INFO(cldev, "Ownership req denied\n");
> > >
> > > why?????
> > >
> > > > +		return;
> > >
> > > No error returned?
> >
> > This is not an error. This means that the CSME firmware is not allowing=
 the
> host to use the WLAN device.
>=20
> Ok, and what can a user do about this?

Not much, but a kernel developer would like to know that it happened.

>=20
> > > > +	if (!netdev) {
> > > > +		IWL_MEI_INFO(cldev, "No netdevice, dropping the Tx
> > > packet\n");
> > >
> > > quiet!!!
> >
> > This is actually not a usual path. A race is happening here...
>=20
> "race" implies "error" which is not "dev_info()" material.
>=20
> > > Or at the least, make this an error so that a user can handle it
> > > properly.  They can do something about this, right?  If not, why did
> > > you just tell them this happened?
> >
> > There isn't much I can do here. The CSME firmware is sending a packet
> > to iwlmei to send on the WLAN link, but the link isn't valid anymore.
>=20
> Then report an error.
>=20
> > > > +		if (le16_to_cpu(hdr.type) !=3D SAP_MSG_DATA_PACKET) {
> > > > +			IWL_MEI_INFO(cldev, "Unsupported message: type
> > > %d, len %d\n",
> > > > +				     le16_to_cpu(hdr.type), len);
> > >
> > > So userspace can spam the kernel log?
> >
> > This is not userspace, this is the CSME firmware.
>=20
> Then make it an error, not an informational message.  Someone needs to se=
e
> it to fix the firmware, right?

Ok.

>=20
> > > Please revisit _ALL_ of your messages you are printing out here, it
> > > feels way way way too noisy, like you got the code up and working
> > > with the debug lines in it and forgot to remove it.
> >
> > [   12.665966] iwlmei 0000:00:16.0-13280904-7792-4fcb-a1aa-5e70cbb1e865=
:
> Got a valid SAP_ME_MSG_START_OK from CSME firmware
> >
> > That's the only line I am printing by default. I can remove it. I can't=
 do better
> than 0 printing.
>=20
> 0 printing would be good.
>=20
> > > > +}
> > > > +
> > > > +#else
> > > > +
> > > > +static void iwl_mei_dbgfs_register(struct iwl_mei *mei) {} static
> > > > +void iwl_mei_dbgfs_unregister(struct iwl_mei *mei) {}
> > >
> > > This type of thing goes in a .h file, you know better.
> >
> > A header file only for less a handful of functions?
>=20
> You already have .h files for this code, why not put it in there?
>=20
> thanks,
>=20
> greg k-h