Message-Id: <20241029082218.10820-1-jinjian.song@fibocom.com>
Date: Tue, 29 Oct 2024 16:22:18 +0800
From: Jinjian Song <jinjian.song@...ocom.com>
To: ryazanov.s.a@...il.com,
	Jinjian Song <jinjian.song@...ocom.com>
Cc: andrew+netdev@...n.ch,
	angelogioacchino.delregno@...labora.com,
	chandrashekar.devegowda@...el.com,
	chiranjeevi.rapolu@...ux.intel.com,
	corbet@....net,
	danielwinkler@...gle.com,
	davem@...emloft.net,
	edumazet@...gle.com,
	haijun.liu@...iatek.com,
	helgaas@...nel.org,
	horms@...nel.org,
	johannes@...solutions.net,
	korneld@...gle.com,
	kuba@...nel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	linux-mediatek@...ts.infradead.org,
	loic.poulain@...aro.org,
	matthias.bgg@...il.com,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	ricardo.martinez@...ux.intel.com
Subject: Re: [net-next v7 2/2] net: wwan: t7xx: Add debug port

From: Sergey Ryazanov <ryazanov.s.a@...il.com>

Hi Sergey,
 
>Hello Jinjian,
>
>On 26.10.2024 12:09, Jinjian Song wrote:
>[skiped]
>> Application can use MIPC (Modem Information Process Center) port
>> to debug antenna tuner or noise profiling through this MTK modem
>> diagnostic interface.
>> 
>> By default, debug ports are not exposed, so using the command
>> to enable or disable debug ports.
>> 
>> Switch on debug port:
>>   - debug: 'echo debug > /sys/bus/pci/devices/${bdf}/t7xx_mode
>> 
>> Switch off debug port:
>>   - normal: 'echo normal > /sys/bus/pci/devices/${bdf}/t7xx_mode
>
>Looks like this part of the message needs an update. Now driver uses a 
>dedicated file for this operation.
>

Yes, please let me update it, thanks.

>> diff --git a/Documentation/networking/device_drivers/wwan/t7xx.rst b/Documentation/networking/device_drivers/wwan/t7xx.rst
>> index f346f5f85f15..6071dee8c186 100644
>> --- a/Documentation/networking/device_drivers/wwan/t7xx.rst
>> +++ b/Documentation/networking/device_drivers/wwan/t7xx.rst
>> @@ -7,12 +7,13 @@
>>   ============================================
>>   t7xx driver for MTK PCIe based T700 5G modem
>>   ============================================
>> -The t7xx driver is a WWAN PCIe host driver developed for linux or Chrome OS platforms
>> -for data exchange over PCIe interface between Host platform & MediaTek's T700 5G modem.
>> -The driver exposes an interface conforming to the MBIM protocol [1]. Any front end
>> -application (e.g. Modem Manager) could easily manage the MBIM interface to enable
>> -data communication towards WWAN. The driver also provides an interface to interact
>> -with the MediaTek's modem via AT commands.
>> +The t7xx driver is a WWAN PCIe host driver developed for linux or Chrome OS
>> +platforms for data exchange over PCIe interface between Host platform &
>> +MediaTek's T700 5G modem.
>> +The driver exposes an interface conforming to the MBIM protocol [1]. Any front
>> +end application (e.g. Modem Manager) could easily manage the MBIM interface to
>> +enable data communication towards WWAN. The driver also provides an interface
>> +to interact with the MediaTek's modem via AT commands.
>
>Thank you for taking care and unifying documentation, still, I believe, 
>this change doesn't belong to this specific patch, what introduced debug 
>ports toggling knob. Could you factor our these formating updating 
>changes into a dedicated patch? E.g. add a new patch "2/3: unify 
>documentation" and make this patch third in the series.
>

Got it, please let me do it.

>> @@ -67,6 +68,28 @@ Write from userspace to set the device mode.
>>   ::
>>     $ echo fastboot_switching > /sys/bus/pci/devices/${bdf}/t7xx_mode
>>   
>> +t7xx_port_mode
>
>I believe we should use the plural form - portS, since this knob 
>controls behaviour of the group of ports.

Fastboot switching is a bit special, this will trigger the WWAN device
reboot to fastboot mode and only a fastboot port in this mode, How about keep it
as before?

>
>And I have one more suggestion. "mode" sounds too generic, can we 
>consider renaming this option to something, what includes more details 
>about the mode. E.g. can we rename this knob to 't7xx_debug_ports' and 
>make it simple boolean (on/off) option?

Yes, rename to 't7xx_debug_ports' and make it boolean is reasonablei, please let
me change it.

>>
>> -static struct attribute *t7xx_mode_attr[] = {
>> +static ssize_t t7xx_port_mode_store(struct device *dev,
>> +				    struct device_attribute *attr,
>> +				    const char *buf, size_t count)
>> +{
>> +	struct t7xx_pci_dev *t7xx_dev;
>> +	struct pci_dev *pdev;
>> +	int index = 0;
>> +
>> +	pdev = to_pci_dev(dev);
>
>This assignment should be done along the variable declaration to make 
>code shorter:

Yes, please let me modify it.

>>
>struct pci_dev *pdev = to_pci_dev(dev);
>
>> +	t7xx_dev = pci_get_drvdata(pdev);
>> +	if (!t7xx_dev)
>> +		return -ENODEV;
>> +
>> +	index = sysfs_match_string(t7xx_port_mode_names, buf);
>> +	if (index == T7XX_DEBUG) {
>> +		t7xx_proxy_port_debug(t7xx_dev, true);
>
>Another one nit picking question. It is unclear what is going to happen 
>after this call. Can we rename this function to something what clearly 
>indicates the desired reaction? E.g. t7xx_proxy_debug_ports_show(...).

After t7xx_proxy_port_debug(t7xx_dev, true) the adb and mipc port will be
shown directly, let me rename to t7xx_proxy_debug_ports_show more clearly.

>> +static ssize_t t7xx_port_mode_show(struct device *dev,
>> +				   struct device_attribute *attr,
>> +				   char *buf)
>> +{
>> +	enum t7xx_port_mode port_mode = T7XX_NORMAL;
>> +	struct t7xx_pci_dev *t7xx_dev;
>> +	struct pci_dev *pdev;
>> +
>> +	pdev = to_pci_dev(dev);
>
>Also should be assigned on declaration.

Yes, please let me modify it.

>>
>> +enum t7xx_port_mode {
>> +	T7XX_NORMAL,
>> +	T7XX_DEBUG,
>> +	T7XX_PORT_MODE_LAST, /* must always be last */
>> +};
>> +
>>   /* struct t7xx_pci_dev - MTK device context structure
>>    * @intr_handler: array of handler function for request_threaded_irq
>>    * @intr_thread: array of thread_fn for request_threaded_irq
>> @@ -94,6 +100,7 @@ struct t7xx_pci_dev {
>>   	struct dentry		*debugfs_dir;
>>   #endif
>>   	u32			mode;
>> +	u32			port_mode;
>
>If we agree to rename the sysfs file to 't7xx_debug_ports', this field 
>can be renamed to something more specific like 'debug_ports_show'.

Yes, let me rename sysfs file to 't7xx_debug_ports' and rename this feild
to 'debug_ports_show'.

>>   
>>   struct t7xx_port {
>> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> index 35743e7de0c3..26d3f57732cc 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
>> @@ -39,6 +39,8 @@
>>   
>>   #define Q_IDX_CTRL			0
>>   #define Q_IDX_MBIM			2
>> +#define Q_IDX_MIPC			2
>
>Are you sure that we should define a new name for the same queue id? Can 
>we just specify Q_IDX_MBIM in the port description or rename Q_IDX_MBIM 
>to Q_IDX_MBIM_MIPC to avoid id names duplication?

Since MBIM and MIPC use the same queue id, please let me rename to Q_IDX_MBIM_MIPC.

>>   
>> +void t7xx_proxy_port_debug(struct t7xx_pci_dev *t7xx_dev, bool show)
>> +{
>> +	struct port_proxy *port_prox = t7xx_dev->md->port_prox;
>> +	struct t7xx_port *port;
>> +	int i;
>> +
>> +	for_each_proxy_port(i, port, port_prox) {
>> +		const struct t7xx_port_conf *port_conf = port->port_conf;
>> +
>> +		if (port_conf->debug && port_conf->ops && port_conf->ops->init) {
>> +			if (show)
>> +				port_conf->ops->init(port);
>> +			else
>> +				port_conf->ops->uninit(port);
>
>I still do not like the assumption that if .init method is defined then 
>the .uninit method is defined too. Is it make sense to compose these 
>checks like below. Mmove the check for .init/.uninit inside and add a 
>comment justifying absense of a check of a current port state.
>
>/* NB: it is safe to call init/uninit multiple times */
>if (port_conf->debug && port_conf->ops) {
>	if (show && port_conf->ops->init)
>		port_conf->ops->init(port);
>	else if (!show && port_conf->ops->uninit)
>		port_conf->ops->uninit(port);
>}


Yes, please let me modify it.


Best Regards,
Jinjian.

Powered by blists - more mailing lists