Message-ID: <15e9b3bd-5d3d-7b44-758f-4013b61e51fa@linux.intel.com>
Date:   Tue, 6 Sep 2022 15:45:16 +0530
From:   "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...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,
        krishna.c.sudi@...el.com, linuxwwan@...el.com,
        Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>,
        Mishra Soumya Prakash <soumya.prakash.mishra@...el.com>
Subject: Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw
 flashing and coredump collection

Thank you Ilpo for reviewing patches.
Will address patch4 & patch5 review comments in v2.

On 8/18/2022 8:03 PM, Ilpo Järvinen wrote:
> On Tue, 16 Aug 2022, m.chetan.kumar@...el.com wrote:
> 
>> From: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
>>
>> This patch brings-in support for t7xx wwan device firmware flashing &
> 
> Just say "Add support for ..."
> 
>> coredump collection using devlink.
>>
>> Driver Registers with Devlink framework.
>> Implements devlink ops flash_update callback that programs modem firmware.
>> 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 then registers with devlink framework.
>> Upon receiving firmware image & partition details driver sends fastboot
>> commands for flashing the firmware.
> 
> Things don't seem to connect well between sentences in this paragraph.
> 
>> In this flow the fastboot command & response gets 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>
>> ---
> 
>> +static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
>> +{
>> +	int ret = 0, read_len;
>> +	struct sk_buff *skb;
>> +
>> +	spin_lock_irq(&port->rx_wq.lock);
>> +	if (skb_queue_empty(&port->rx_skb_list)) {
>> +		ret = wait_event_interruptible_locked_irq(port->rx_wq,
>> +							  !skb_queue_empty(&port->rx_skb_list));
>> +		if (ret == -ERESTARTSYS) {
>> +			spin_unlock_irq(&port->rx_wq.lock);
>> +			return -EINTR;
>> +		}
>> +	}
>> +	skb = skb_dequeue(&port->rx_skb_list);
>> +	spin_unlock_irq(&port->rx_wq.lock);
>> +
>> +	read_len = count > skb->len ? skb->len : count;
> 
> max_t()
> 
>> +	memcpy(buf, skb->data, read_len);
>> +	dev_kfree_skb(skb);
>> +
>> +	return ret ? ret : read_len;
> 
> Can ret actually be non-zero here since -ERESTARTSYS is covered above?
> 
>> +}
>> +
>> +static int t7xx_devlink_port_write(struct t7xx_port *port, const char *buf, size_t count)
>> +{
>> +	const struct t7xx_port_conf *port_conf = port->port_conf;
>> +	size_t actual_count;
>> +	struct sk_buff *skb;
>> +	int ret, txq_mtu;
>> +
>> +	txq_mtu = t7xx_get_port_mtu(port);
>> +	if (txq_mtu < 0)
>> +		return -EINVAL;
>> +
>> +	actual_count = count > txq_mtu ? txq_mtu : count;
> 
> max_t()
> 
>> +	skb = __dev_alloc_skb(actual_count, GFP_KERNEL);
>> +	if (!skb)
>> +		return -ENOMEM;
>> +
>> +	skb_put_data(skb, buf, actual_count);
>> +	ret = t7xx_port_send_raw_skb(port, skb);
>> +	if (ret) {
>> +		dev_err(port->dev, "write error on %s, size: %zu, ret: %d\n",
>> +			port_conf->name, actual_count, ret);
>> +		dev_kfree_skb(skb);
>> +		return ret;
>> +	}
>> +
>> +	return actual_count;
>> +}
>> +
>> +static int t7xx_devlink_fb_handle_response(struct t7xx_port *port, int *data)
>> +{
>> +	int ret = 0, index = 0, return_data = 0, read_bytes;
> 
> I'd move return_data declaration into block where it is used. I don't
> think it needs to be initialized or does the compiler complain about it?
> 
> 
>> +	char status[T7XX_FB_RESPONSE_SIZE + 1];
>> +
>> +	while (index < T7XX_FB_RESP_COUNT) {
>> +		index++;
> 
> for (index = 0; index < T7XX_FB_RESP_COUNT; index++) {
> 
>> +		read_bytes = t7xx_devlink_port_read(port, status, T7XX_FB_RESPONSE_SIZE);
>> +		if (read_bytes < 0) {
>> +			dev_err(port->dev, "status read failed");
> 
> Printing "read failed" for -ERESTARTSYS/-EINTR??
> 
>> +static int t7xx_devlink_fb_download(struct t7xx_port *port, const u8 *buf, size_t size)
>> +{
>> +	int ret;
>> +
>> +	if (size <= 0 || size > SIZE_MAX) {
>> +		dev_err(port->dev, "file is too large to download");
>> +		return -EINVAL;
> 
> The if condition is effectively if (!size) because unsigned cannot be <0
> nor can size_t be > (~(size_t)0). Given that, the error message is pretty
> bogus. I'd tend to think returning just -EINVAL in case of !size w/o any
> printingis sufficient here.
> 
>> +static int t7xx_devlink_fb_get_core(struct t7xx_port *port)
>> +{
>> +	struct t7xx_devlink_region_info *mrdump_region;
>> +	char mrdump_complete_event[T7XX_FB_EVENT_SIZE];
>> +	u32 mrd_mb = T7XX_MRDUMP_SIZE / (1024 * 1024);
>> +	struct t7xx_devlink *dl = port->dl;
>> +	int clen, dlen = 0, result = 0;
> 
> No need to init dlen.
> 
>> +	unsigned long long zipsize = 0;
>> +	char mcmd[T7XX_FB_MCMD_SIZE];
>> +	size_t offset_dlen = 0;
>> +	char *mdata;
>> +
>> +	set_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> +	mdata = kmalloc(T7XX_FB_MDATA_SIZE, GFP_KERNEL);
>> +	if (!mdata) {
>> +		result = -ENOMEM;
>> +		goto get_core_exit;
>> +	}
>> +
>> +	mrdump_region = dl->dl_region_info[T7XX_MRDUMP_INDEX];
>> +	mrdump_region->dump = vmalloc(mrdump_region->default_size);
>> +	if (!mrdump_region->dump) {
>> +		kfree(mdata);
>> +		result = -ENOMEM;
>> +		goto get_core_exit;
>> +	}
> 
> Error handling/cleanup paths need to be cleaned up here.
> 
>> +	result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_MRDUMP, port, NULL);
>> +	if (result) {
>> +		dev_err(port->dev, "%s command failed\n", T7XX_FB_CMD_OEM_MRDUMP);
>> +		vfree(mrdump_region->dump);
>> +		kfree(mdata);
>> +		goto get_core_exit;
>> +	}
> 
> Ditto.
> 
>> +	while (mrdump_region->default_size > offset_dlen) {
>> +		clen = t7xx_devlink_port_read(port, mcmd, sizeof(mcmd));
>> +		if (clen == strlen(T7XX_FB_CMD_RTS) &&
>> +		    (!strncmp(mcmd, T7XX_FB_CMD_RTS, strlen(T7XX_FB_CMD_RTS)))) {
>> +			memset(mdata, 0, T7XX_FB_MDATA_SIZE);
>> +			dlen = 0;
> 
> Unnecessary assignment.
> 
>> +			memset(mcmd, 0, sizeof(mcmd));
>> +			clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_CTS);
>> +
>> +			if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
>> +				dev_err(port->dev, "write for _CTS failed:%d\n", clen);
>> +				goto get_core_free_mem;
>> +			}
>> +
>> +			dlen = t7xx_devlink_port_read(port, mdata, T7XX_FB_MDATA_SIZE);
>> +			if (dlen <= 0) {
>> +				dev_err(port->dev, "read data error(%d)\n", dlen);
>> +				goto get_core_free_mem;
>> +			}
>> +
>> +			zipsize += (unsigned long long)(dlen);
>> +			memcpy(mrdump_region->dump + offset_dlen, mdata, dlen);
>> +			offset_dlen += dlen;
> 
> Why both offset_dlen and zipsize???
> 
>> +			memset(mcmd, 0, sizeof(mcmd));
>> +			clen = snprintf(mcmd, sizeof(mcmd), "%s", T7XX_FB_CMD_FIN);
>> +			if (t7xx_devlink_port_write(port, mcmd, clen) != clen) {
>> +				dev_err(port->dev, "%s: _FIN failed, (Read %05d:%05llu)\n",
>> +					__func__, clen, zipsize);
> 
> Printing __func__ probably isn't that helpful here.
> 
>> +				goto get_core_free_mem;
>> +			}
>> +		} else if ((clen == strlen(T7XX_FB_RESP_MRDUMP_DONE)) &&
>> +			  (!strncmp(mcmd, T7XX_FB_RESP_MRDUMP_DONE,
>> +				    strlen(T7XX_FB_RESP_MRDUMP_DONE)))) {
> 
> strcmp()
> 
>> +			dev_dbg(port->dev, "%s! size:%zd\n", T7XX_FB_RESP_MRDUMP_DONE, offset_dlen);
>> +			mrdump_region->actual_size = offset_dlen;
>> +			snprintf(mrdump_complete_event, sizeof(mrdump_complete_event),
>> +				 "%s size=%zu", T7XX_UEVENT_MRDUMP_READY, offset_dlen);
>> +			t7xx_uevent_send(dl->dev, mrdump_complete_event);
>> +			kfree(mdata);
>> +			result = 0;
>> +			goto get_core_exit;
>> +		} else {
>> +			dev_err(port->dev, "getcore protocol error (read len %05d)\n", clen);
>> +			goto get_core_free_mem;
>> +		}
>> +	}
>> +
>> +	dev_err(port->dev, "mrdump exceeds %uMB size. Discarded!", mrd_mb);
>> +	t7xx_uevent_send(port->dev, T7XX_UEVENT_MRD_DISCD);
>> +
>> +get_core_free_mem:
>> +	kfree(mdata);
>> +	vfree(mrdump_region->dump);
>> +	clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> +	return -EPROTO;
>> +
>> +get_core_exit:
>> +	clear_bit(T7XX_MRDUMP_STATUS, &dl->status);
>> +	return result;
> 
> Error handling/cleanup paths need to be cleaned up here.
> 
>> +}
>> +
>> +static int t7xx_devlink_fb_dump_log(struct t7xx_port *port)
>> +{
>> +	struct t7xx_devlink_region_info *lkdump_region;
>> +	char lkdump_complete_event[T7XX_FB_EVENT_SIZE];
>> +	struct t7xx_devlink *dl = port->dl;
>> +	int dlen, datasize = 0, result;
>> +	size_t offset_dlen = 0;
>> +	u8 *data;
>> +
>> +	set_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> +	result = t7xx_devlink_fb_raw_command(T7XX_FB_CMD_OEM_LKDUMP, port, &datasize);
>> +	if (result) {
>> +		dev_err(port->dev, "%s command returns failure\n", T7XX_FB_CMD_OEM_LKDUMP);
>> +		goto lkdump_exit;
>> +	}
>> +
>> +	lkdump_region = dl->dl_region_info[T7XX_LKDUMP_INDEX];
>> +	if (datasize > lkdump_region->default_size) {
>> +		dev_err(port->dev, "lkdump size is more than %dKB. Discarded!",
>> +			T7XX_LKDUMP_SIZE / 1024);
>> +		t7xx_uevent_send(dl->dev, T7XX_UEVENT_LKD_DISCD);
>> +		result = -EPROTO;
>> +		goto lkdump_exit;
>> +	}
>> +
>> +	data = kzalloc(datasize, GFP_KERNEL);
>> +	if (!data) {
>> +		result = -ENOMEM;
>> +		goto lkdump_exit;
>> +	}
>> +
>> +	lkdump_region->dump = vmalloc(lkdump_region->default_size);
>> +	if (!lkdump_region->dump) {
>> +		kfree(data);
>> +		result = -ENOMEM;
>> +		goto lkdump_exit;
> 
> Ditto.
> 
>> +	}
>> +
>> +	while (datasize > 0) {
>> +		dlen = t7xx_devlink_port_read(port, data, datasize);
>> +		if (dlen <= 0) {
>> +			dev_err(port->dev, "lkdump read error ret = %d", dlen);
>> +			kfree(data);
>> +			result = -EPROTO;
>> +			goto lkdump_exit;
> 
> Ditto.
> 
>> +		}
>> +
>> +		memcpy(lkdump_region->dump + offset_dlen, data, dlen);
>> +		datasize -= dlen;
>> +		offset_dlen += dlen;
>> +	}
>> +
>> +	dev_dbg(port->dev, "LKDUMP DONE! size:%zd\n", offset_dlen);
>> +	lkdump_region->actual_size = offset_dlen;
>> +	snprintf(lkdump_complete_event, sizeof(lkdump_complete_event), "%s size=%zu",
>> +		 T7XX_UEVENT_LKDUMP_READY, offset_dlen);
>> +	t7xx_uevent_send(dl->dev, lkdump_complete_event);
>> +	kfree(data);
>> +	clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> +	return t7xx_devlink_fb_handle_response(port, NULL);
>> +
>> +lkdump_exit:
>> +	clear_bit(T7XX_LKDUMP_STATUS, &dl->status);
>> +	return result;
>> +}
> 
> 
>> +/* To create regions for dump files */
>> +static int t7xx_devlink_create_region(struct t7xx_devlink *dl)
>> +{
>> +	struct devlink_region_ops *region_ops;
>> +	int rc, i;
>> +
>> +	region_ops = dl->dl_region_ops;
>> +	for (i = 0; i < T7XX_TOTAL_REGIONS; i++) {
> 
> Perhaps this is a matter of taste thing but I'd use
> ARRAY_SIZE(t7xx_devlink_region_list).
> 
>> +		region_ops[i].name = t7xx_devlink_region_list[i].region_name;
>> +		region_ops[i].snapshot = t7xx_devlink_region_snapshot;
>> +		region_ops[i].destructor = vfree;
>> +		dl->dl_region[i] =
>> +		devlink_region_create(dl->dl_ctx, &region_ops[i], T7XX_MAX_SNAPSHOTS,
>> +				      t7xx_devlink_region_list[i].default_size);
> 
> Indentation.
> 
>> +		if (IS_ERR(dl->dl_region[i])) {
>> +			rc = PTR_ERR(dl->dl_region[i]);
>> +			dev_err(dl->dev, "devlink region fail,err %d", rc);
> 
> Please fix message formatting.
> 
>> +	rc = t7xx_devlink_create_region(dl);
>> +	if (rc) {
>> +		dev_err(dl->dev, "devlink region creation failed, rc %d", rc);
> 
> Since this is user visible error, "rc" should be replaced with something
> meaningful.
> 
> 
>> +static int t7xx_devlink_init(struct t7xx_port *port)
>> +{
>> +	struct t7xx_devlink_work *dl_work;
>> +	struct workqueue_struct *wq;
>> +
>> +	dl_work = kmalloc(sizeof(*dl_work), GFP_KERNEL);
>> +	if (!dl_work)
>> +		return -ENOMEM;
>> +
>> +	wq = create_workqueue("t7xx_devlink");
>> +	if (!wq) {
>> +		kfree(dl_work);
>> +		dev_err(port->dev, "create_workqueue failed\n");
>> +		return -ENODATA;
>> +	}
>> +
>> +	INIT_WORK(&dl_work->work, t7xx_devlink_work_handler);
>> +	dl_work->port = port;
>> +	port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>> +
>> +	if (!t7xx_devlink_region_init(port, dl_work, wq))
>> +		return -ENOMEM;
> 
> Leaks?
> 
>> +static int t7xx_devlink_disable_chl(struct t7xx_port *port)
>> +{
>> +	spin_lock(&port->port_update_lock);
>> +	port->chan_enable = false;
>> +	spin_unlock(&port->port_update_lock);
>> +
>> +	return 0;
>> +}
> 
> This is identical to t7xx_port_wwan_disable_chl().
> 
>> +struct t7xx_devlink_region_info {
>> +	char region_name[T7XX_MAX_REGION_NAME_LENGTH];
>> +	u32 default_size;
>> +	u32 actual_size;
>> +	u32 entry;
> 
> entry seems pretty useless, used only to show an index within a debug
> message.
> 
>> diff --git a/drivers/net/wwan/t7xx/t7xx_reg.h b/drivers/net/wwan/t7xx/t7xx_reg.h
>> index 60e025e57baa..3a758bf79a4e 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_reg.h
>> +++ b/drivers/net/wwan/t7xx/t7xx_reg.h
>> @@ -101,11 +101,17 @@ enum t7xx_pm_resume_state {
>>   	PM_RESUME_REG_STATE_L2_EXP,
>>   };
>>   
>> +enum host_event_e {
>> +	HOST_EVENT_INIT = 0,
>> +	FASTBOOT_DL_NOTY = 0x3,
> 
> NOTIFY?
> 
> 

-- 
Chetan

Powered by blists - more mailing lists