netdev - Re: [PATCH v4 net-next 4/5] net: wwan: t7xx: Enable devlink based fw flashing and coredump collection

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]
Date:   Wed, 18 Jan 2023 21:25:52 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
cc:     Netdev <netdev@...r.kernel.org>, kuba@...nel.org,
        davem@...emloft.net, johannes@...solutions.net,
        ryazanov.s.a@...il.com, loic.poulain@...aro.org,
        ricardo.martinez@...ux.intel.com,
        chiranjeevi.rapolu@...ux.intel.com, haijun.liu@...iatek.com,
        edumazet@...gle.com, pabeni@...hat.com,
        chandrashekar.devegowda@...el.com, linuxwwan@...el.com,
        linuxwwan_5g@...el.com,
        Mishra Soumya Prakash <soumya.prakash.mishra@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH v4 net-next 4/5] net: wwan: t7xx: Enable devlink based
 fw flashing and coredump collection

On Wed, 18 Jan 2023, Kumar, M Chetan wrote:

> Hi Ilpo,
> Thank you for the feedback.
> 
> On 1/17/2023 7:37 PM, Ilpo Järvinen wrote:
> > On Mon, 16 Jan 2023, m.chetan.kumar@...ux.intel.com wrote:
> > 
> > > From: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
> > > 
> > > Adds support for t7xx wwan device firmware flashing & coredump collection
> > > using devlink.
> > > 
> > > 1> Driver Registers with Devlink framework.
> > > 2> Implements devlink ops flash_update callback that programs modem fw.
> > > 3> Creates region & snapshot required for device coredump log collection.
> > > 
> > > On early detection of wwan device in fastboot mode driver sets up CLDMA0
> > > HW
> > > tx/rx queues for raw data transfer and then registers to devlink
> > > framework.
> > > On user space application issuing command for firmware update the driver
> > > sends fastboot flash command & firmware to program NAND.
> > > 
> > > In flashing procedure the fastboot command & response are exchanged
> > > between
> > > driver and device. Once firmware flashing is success completion status is
> > > reported to user space application.
> > > 
> > > Below is the devlink command usage for firmware flashing
> > > 
> > > $devlink dev flash pci/$BDF file ABC.img component ABC
> > > 
> > > Note: ABC.img is the firmware to be programmed to "ABC" partition.
> > > 
> > > In case of coredump collection when wwan device encounters an exception
> > > it reboots & stays in fastboot mode for coredump collection by host
> > > driver.
> > > On detecting exception state driver collects the core dump, creates the
> > > devlink region & reports an event to user space application for dump
> > > collection. The user space application invokes devlink region read command
> > > for dump collection.
> > > 
> > > Below are the devlink commands used for coredump collection.
> > > 
> > > devlink region new pci/$BDF/mr_dump
> > > devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
> > > devlink region del pci/$BDF/mr_dump snapshot $ID
> > > 
> > > Signed-off-by: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
> > > Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>
> > > Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@...el.com>
> > > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> > > --

> > > +	for (i = 0; i < total_part; i++) {
> > 
> > The whole operation below is quite fancy, I'd add some comment telling the
> > intent.
> 
> Device returns firmware name & version in string format. Using below logic to
> decode it.
>
> Will add some comment.
> 
> > 
> > > +		part_name = strsep(&data, ",");
> > > +		ver = strsep(&data, ",");
> > 
> > Can ver become NULL here?
> 
> It should not be the case. As part of component fw version query device is
> expected to send the complete list for fw components.
> 
> On safer note will add NULL check.
> 
> > 
> > > +		ver_len = strlen(ver);
> > > +		if (ver[ver_len - 2] == 0x5C && ver[ver_len - 1] == 0x6E)
> > > +			ver[ver_len - 4] = '\0';
> > 
> > Is ver_len guaranteed to be large enough?
> 
> fw version query response message will not cross 512 bytes.
> It is aligned with device implementation.

I meant the other way around, is ver_len guaranteed to large enough that 
it is safe to do ver_len - 4 (or even -1).

> > > +		ret = devlink_info_version_running_put_ext(req, part_name,
> > > ver,
> > > +
> > > DEVLINK_INFO_VERSION_TYPE_COMPONENT);
> > > +	}
> > > +
> > > +err_clear_bit:
> > > +	clear_bit(T7XX_GET_INFO, &dl->status);
> > > +	kfree(data);
> > > +	return ret;
> > > +}

> > > +static void t7xx_devlink_uninit(struct t7xx_port *port)
> > > +{
> > > +	struct t7xx_devlink *dl = port->t7xx_dev->dl;
> > > +	int i;
> > > +
> > > +	vfree(dl->regions[T7XX_MRDUMP_INDEX].buf);
> > > +
> > > +	dl->mode = T7XX_NORMAL_MODE;
> > > +	destroy_workqueue(dl->wq);
> > > +
> > > +	BUILD_BUG_ON(ARRAY_SIZE(t7xx_devlink_region_infos) >
> > > ARRAY_SIZE(dl->regions));
> > 
> > The same BUILD_BUG_ON again? Maybe just make a single static_assert()
> > outside of the functions.
> 
> Should i change it as below ? please suggest.
> 
> static_assert(ARRAY_SIZE(t7xx_devlink_region_infos) ==
>               (sizeof(typeof_member(struct t7xx_devlink, regions)) /
>                sizeof(struct t7xx_devlink_region)));
> static void t7xx_devlink_uninit(struct t7xx_port *port)
> {
> ..

I see, it's not that easy so perhaps just leave it as is.

I guess something like this might work but seems bit hacky to me 
(untested):

static_assert(ARRAY_SIZE(t7xx_devlink_region_infos) ==
	      ARRAY_SIZE(((struct t7xx_devlink *)NULL)->regions));

-- 
 i.