netdev - Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework

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: Sat,  5 Aug 2023 18:25:40 +0800
From: Jinjian Song <songjinjian@...mail.com>
To: jiri@...nulli.us
Cc: chandrashekar.devegowda@...el.com,
	chiranjeevi.rapolu@...ux.intel.com,
	danielwinkler@...gle.com,
	davem@...emloft.net,
	edumazet@...gle.com,
	haijun.liu@...iatek.com,
	ilpo.jarvinen@...ux.intel.com,
	jesse.brandeburg@...el.com,
	jinjian.song@...ocom.com,
	johannes@...solutions.net,
	kuba@...nel.org,
	linuxwwan@...el.com,
	linuxwwan_5g@...el.com,
	loic.poulain@...aro.org,
	m.chetan.kumar@...ux.intel.com,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	ricardo.martinez@...ux.intel.com,
	ryazanov.s.a@...il.com,
	songjinjian@...mail.com,
	soumya.prakash.mishra@...el.com
Subject: Re: [net-next 2/6] net: wwan: t7xx: Driver registers with Devlink framework

>>+static const struct devlink_param t7xx_devlink_params[] = {
>>+	DEVLINK_PARAM_DRIVER(T7XX_DEVLINK_PARAM_ID_FASTBOOT,
>>+			     "fastboot", DEVLINK_PARAM_TYPE_BOOL,
>>+			     BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>+			     NULL, NULL, NULL),
>>+};
>
>Please have the param introduction in a separate file.
>
>Just to mention it right here, this param smells very oddly to me.
>

Thanks for your review, I will add introduction.

>>+static int t7xx_devlink_reload_down(struct devlink *devlink, bool netns_change,
>>+				    enum devlink_reload_action action,
>>+				    enum devlink_reload_limit limit,
>>+				    struct netlink_ext_ack *extack)
>>+{
>>+	return 0;
>>+}
>>+
>>+static int t7xx_devlink_reload_up(struct devlink *devlink,
>>+				  enum devlink_reload_action action,
>>+				  enum devlink_reload_limit limit,
>>+				  u32 *actions_performed,
>>+				  struct netlink_ext_ack *extack)
>>+{
>>+	return 0;
>>+}
>>+
>>+static int t7xx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
>>+				 struct netlink_ext_ack *extack)
>>+{
>>+	return 0;
>>+}
>
>Don't put the stub functions here. Introduce them when you need them.

Thanks for your review, I have split devlink falsh to devlink register and devlink flash
as suggestion on v5 before, I want to move this functions to 
'device firmware flashing using devlink' patch, is that ok?

>>+static int t7xx_devlink_init(struct t7xx_port *port)
>>+{
>>+	struct t7xx_devlink *dl = port->t7xx_dev->dl;
>>+
>>+	port->rx_length_th = T7XX_MAX_QUEUE_LENGTH;
>>+
>>+	dl->mode = T7XX_NORMAL_MODE;
>>+	dl->status = T7XX_DEVLINK_IDLE;
>
>? What is this supposed to mean? Looks quite wrong.

Thanks for your review, modem register devlink framework to do firmware flashing and core
dump collection, we define mode to identify modem mode T7XX_NORMAL_MODE, T7XX_FB_DL_MODE
(modem fastboot download mode), T7XX_FB_DUMP_MODE(modem fastboot coredump modem)

status used to define the current devlink progress status T7XX_NORMAL_MODE<->T7XX_DEVLINK_IDLE,
T7XX_FB_DL_MODE<->T7XX_FLASH_STATUS, devlink ops info_get<->T7XX_GET_INFO, T7XX_FB_DUMP_MODE<->
(T7XX_LKDUMP_STATUS,T7XX_MRDUMP_STATUS)

>>+	dl->port = port;
>>+
>>+	return 0;
>>+}
>>+
>>+static int t7xx_devlink_enable_chl(struct t7xx_port *port)
>>+{
>>+	t7xx_port_enable_chl(port);
>>+
>>+	return 0;
>>+}
>>+
>>+struct port_ops devlink_port_ops = {
>
>Could you reneame this to me less confusing? Looks like this should be
>related to devlink_port, but actually it is not.
>
>Could you please describe what exatly is the "port" entity for your
>driver? What it represents?

Thanks for your review, this port ops used to define modem port witch used to 
flash firmware or collect core dump,devlink command will use this port config to send firmware data
to modem or recieve coredump data from modem.

The "port" mapping to the modem data channel, which used for different functions to comunicate with
modem, all the port configs defined in t7xx_port_proxy.c t7xx_port_config, devlink flash/regions will send
firmware to modem and get core dump info use this "port" config, so we need to define here.

Maybe I can define this port name to flash_dump_port?

>>+	.init = &t7xx_devlink_init,
>>+	.recv_skb = &t7xx_port_enqueue_skb,
>>+	.uninit = &t7xx_devlink_uninit,
>>+	.enable_chl = &t7xx_devlink_enable_chl,
>>+	.disable_chl = &t7xx_port_disable_chl,
>>+};