Re: [PATCH v3] staging: wilc1000: New cfg packet format in handle_set_wfi_drv_handler

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Apr 11, 2017 at 10:11:43PM +0530, Aditya Shankar wrote:
> Change the config packet format used in handle_set_wfi_drv_handler()
> to align the host driver with the new format used in the wilc firmware.
> 
> The change updates the format in which the host driver provides the
> firmware with the drv_handler index and also uses two new
> fields viz. "mode" and 'name" in the config packet along with this index
> to directly provide details about the interface and its mode to the
> firmware instead of having multiple if-else statements in the host driver
> to decide which interface to configure.
> 
> Signed-off-by: Aditya Shankar <aditya.shankar@microchip.com>
> Reviewed-by: Arend Van Spriel <arend.vanspriel@broadcom.com>
> ---
> Change in v2: Fix build warning
> Change in v3: Address review comments from v2
> ---
>  drivers/staging/wilc1000/host_interface.c         | 48 ++++++++++++++++++-----
>  drivers/staging/wilc1000/host_interface.h         |  9 ++++-
>  drivers/staging/wilc1000/linux_wlan.c             | 37 +++++------------
>  drivers/staging/wilc1000/wilc_wfi_cfgoperations.c |  2 +-
>  drivers/staging/wilc1000/wilc_wfi_netdevice.h     |  1 +
>  drivers/staging/wilc1000/wilc_wlan_if.h           |  2 +-
>  6 files changed, 59 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index c3a8af0..7352488 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -334,14 +334,39 @@ static void handle_set_wfi_drv_handler(struct wilc_vif *vif,
>  {
>  	int ret = 0;
>  	struct wid wid;
> +	u8 *currbyte, *buffer;
> +	struct host_if_drv *hif_drv = NULL;
> +
> +	if (!vif->hif_drv)
> +		return;
> +
> +	if (!hif_drv_handler)
> +		return;
> +
> +	hif_drv	= vif->hif_drv;
> +	buffer = kzalloc(DRV_HANDLER_SIZE, GFP_ATOMIC);
> +
> +	if (!buffer)
> +		return;

Test on the line right after allocation.

And there is no way to return an error from this function?  That seems
bad.

And why do you need GFP_ATOMIC?

And finally, where do you free this buffer?

> @@ -3449,8 +3477,10 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length)
>  
>  	id = ((buffer[length - 4]) | (buffer[length - 3] << 8) | (buffer[length - 2] << 16) | (buffer[length - 1] << 24));
>  	vif = wilc_get_vif_from_idx(wilc, id);
> +
>  	if (!vif)
>  		return;
> +
>  	hif_drv = vif->hif_drv;
>  
>  	if (!hif_drv || hif_drv == terminated_handle)	{

Unneeded whitespace changes.  Don't do that in a patch that does other
things.

thanks,

greg k-h