netdev - Re: [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration

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] [day] [month] [year] [list]

Date:   Mon, 20 Sep 2021 09:53:02 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
Cc:     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,
        m.chetan.kumar@...el.com, linuxwwan@...el.com
Subject: Re: [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration

Small style nits.

On Sun, Sep 19, 2021 at 10:56:18PM +0530, M Chetan Kumar wrote:
> +static int ipc_devlink_flash_update(struct devlink *devlink,
> +				    struct devlink_flash_update_params *params,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct iosm_devlink *ipc_devlink = devlink_priv(devlink);
> +	enum iosm_flash_comp_type fls_type;
> +	u32 rc = -EINVAL;

This should be an int.

> +	u8 *mdm_rsp;
> +
> +	if (!params->component)
> +		return rc;

It's more readable to return literals.  "return -EINVAL;"

> +
> +	mdm_rsp = kzalloc(IOSM_EBL_DW_PACK_SIZE, GFP_KERNEL);
> +	if (!mdm_rsp)
> +		return -ENOMEM;
> +
> +	fls_type = ipc_devlink_get_flash_comp_type(params->component,
> +						   strlen(params->component));
> +
> +	switch (fls_type) {
> +	case FLASH_COMP_TYPE_PSI:
> +		rc = ipc_flash_boot_psi(ipc_devlink, params->fw);
> +		break;
> +	case FLASH_COMP_TYPE_EBL:
> +		rc = ipc_flash_boot_ebl(ipc_devlink, params->fw);
> +		if (!rc)

Always do error handling.  Never do success handling.  If you do:

	if (rc)
		break;

> +			rc = ipc_flash_boot_set_capabilities(ipc_devlink,
> +							     mdm_rsp);

Then this fits on one line.

		rc = ipc_flash_boot_set_capabilities(ipc_devlink, mdm_rsp);


> +		if (!rc)
> +			rc = ipc_flash_read_swid(ipc_devlink, mdm_rsp);
> +		break;
> +	case FLASH_COMP_TYPE_FLS:
> +		rc = ipc_flash_send_fls(ipc_devlink, params->fw, mdm_rsp);
> +		break;
> +	default:
> +		devlink_flash_update_status_notify(devlink, "Invalid component",
> +						   params->component, 0, 0);
> +		break;
> +	}
> +
> +	if (!rc)
> +		devlink_flash_update_status_notify(devlink, "Flashing success",
> +						   params->component, 0, 0);
> +	else
> +		devlink_flash_update_status_notify(devlink, "Flashing failed",
> +						   params->component, 0, 0);
> +
> +	kfree(mdm_rsp);
> +	return rc;
> +}

[ snip ]

> +static int ipc_devlink_coredump_snapshot(struct devlink *dl,
> +					 const struct devlink_region_ops *ops,
> +					 struct netlink_ext_ack *extack,
> +					 u8 **data)
> +{
> +	struct iosm_devlink *ipc_devlink = devlink_priv(dl);
> +	struct iosm_coredump_file_info *cd_list = ops->priv;
> +	u32 region_size;
> +	int rc;
> +
> +	dev_dbg(ipc_devlink->dev, "Region:%s, ID:%d", ops->name,
> +		cd_list->entry);
> +	region_size = cd_list->default_size;
> +	rc = ipc_coredump_collect(ipc_devlink, data, cd_list->entry,
> +				  region_size);
> +	if (rc) {
> +		dev_err(ipc_devlink->dev, "Fail to create snapshot,err %d", rc);
> +		goto coredump_collect_err;
> +	}
> +
> +	/* Send coredump end cmd indicating end of coredump collection */
> +	if (cd_list->entry == (IOSM_NOF_CD_REGION - 1))
> +		ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
> +
> +	return rc;

Return literals if possible.

	return 0;

Put a blank line after the "return 0;"

> +coredump_collect_err:
> +	ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
> +	return rc;
> +}


[ snip ]

> +/**
> + * struct iosm_rpsi_cmd - Structure for RPSI Command
> + * @param:      Used to calculate CRC
> + * @cmd:        Stores the RPSI command
> + * @crc:        Stores the CRC value
> + */
> +struct iosm_rpsi_cmd {
> +	union iosm_rpsi_param_u param;
> +	__le16	cmd;
> +	__le16	crc;
> +};
> +
> +/**
> + * ipc_devlink_init - To initialize the devlink to IOSM driver
> + * @ipc_imem:	Pointer to struct iosm_imem
> + *
> + * Returns:	Pointer to iosm_devlink on success and NULL on failure
> + */

These function comments should go in the .c file instead of the .h file.

> +struct iosm_devlink *ipc_devlink_init(struct iosm_imem *ipc_imem);
> +
> +/**
> + * ipc_devlink_deinit - To unintialize the devlink from IOSM driver.
> + * @ipc_devlink:	Devlink instance
> + */

Same.

> +void ipc_devlink_deinit(struct iosm_devlink *ipc_devlink);
> +
> +/**
> + * ipc_devlink_send_cmd - Send command to Modem
> + * @ipc_devlink: Pointer to struct iosm_devlink
> + * @cmd:	 Command to be sent to modem
> + * @entry:	 Command entry number
> + *
> + * Returns:	 0 on success and failure value on error
> + */

Same.

> +int ipc_devlink_send_cmd(struct iosm_devlink *ipc_devlink, u16 cmd, u32 entry);
> +
> +#endif /* _IOSM_IPC_DEVLINK_H */

regards,
dan carpenter