Message-ID: <20200121080152.GF21151@kadam>
Date:   Tue, 21 Jan 2020 11:01:52 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Vladimir Stankovic <vladimir.stankovic@...playlink.com>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        Petar Kovacevic <petar.kovacevic@...playlink.com>,
        Nikola Simic <nikola.simic@...playlink.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Marko Miljkovic <marko.miljkovic@...playlink.com>,
        Stefan Lugonjic <stefan.lugonjic@...playlink.com>
Subject: Re: staging: Add MA USB Host driver

This code is not terrible.  It would have helped a lot if you had
run it through checkpatch --strict.

This driver initializes most local variables to zero which disables
GCC's uninitialized error variable checking and generally makes the code
harder to understand.  Try to remove this as much as you can.

On Mon, Jan 20, 2020 at 09:30:43AM +0000, Vladimir Stankovic wrote:
> +++ b/drivers/staging/mausb_host/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# Copyright (c) 2019 - 2020 DisplayLink (UK) Ltd.
> +#
> +# This file is subject to the terms and conditions of the GNU General Public
> +# License v2. See the file COPYING in the main directory of this archive for
> +# more details.
> +#
> +
> +config HOST_MAUSB
> +	bool "MA-USB Host Driver"
> +	depends on USB=y

Why can't HOST_MAUSB be built as a module?

> +	default y

It should default to disabled.


> +static int mausb_insert_usb_device(struct mausb_dev *mdevs,
> +			    struct mausb_usb_device_ctx *usb_device)
> +{
> +	struct rb_node **new_node = &(mdevs->usb_devices.rb_node),
> +		       *parent = NULL;

This is another this which the code has all over.  Split these up into
two lines.

	struct rb_node **new_node = &mdevs->usb_devices.rb_node;
	struct rb_node *parent = NULL;

Search for all the lines that end in a comma and fix them.


> +static int mausb_hcd_start(struct usb_hcd *hcd)
> +{
> +	mausb_pr_info("");

There is too much debug output.  Here we can use ftrace to get this
information.  A lot of the warning messages are not clear.  One just
says "Fragmentation" without telling the user what to do.  I guess
search for quotes and think about rephrasing or deleting stuff.

> +
> +	hcd->power_budget = 0;
> +	hcd->uses_new_polling = 1;
> +	return 0;
> +}

[ snip ]

> +static int mausb_drop_endpoint(struct usb_hcd *hcd, struct usb_device *dev,
> +			struct usb_host_endpoint *endpoint)
> +{
> +	int8_t	port_number = 0;
> +	int	status	    = 0,
> +		retries	    = 0;
> +	struct hub_ctx		    *hub = (struct hub_ctx *)hcd->hcd_priv;
> +	struct mausb_device	    *ma_dev;
> +	struct mausb_usb_device_ctx *usb_device_ctx;
> +	struct mausb_endpoint_ctx   *endpoint_ctx = endpoint->hcpriv;
> +	unsigned long flags;
> +
> +	port_number = get_root_hub_port_number(dev);
> +
> +	if (port_number < 0 || port_number >= NUMBER_OF_PORTS) {
> +		mausb_pr_info("port_number out of range, port_number=%x",
> +			      port_number);
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&mhcd->lock, flags);
> +	ma_dev = hub->ma_devs[port_number].ma_dev;
> +	spin_unlock_irqrestore(&mhcd->lock, flags);
> +
> +	if (!ma_dev) {
> +		mausb_pr_err("MAUSB device not found on port_number=%d",
> +			     port_number);
> +		return -ENODEV;
> +	}
> +
> +	usb_device_ctx =
> +	    mausb_find_usb_device(&hub->ma_devs[port_number], dev);
> +
> +	if (!endpoint_ctx) {
> +		mausb_pr_err("Endpoint context doesn't exist");
> +		return 0;
> +	}
> +	if (!usb_device_ctx) {
> +		mausb_pr_err("Usb device context doesn't exist");
> +		return 0;
> +	}
> +
> +	mausb_pr_info("Start dropping ep_handle=%#x, dev_handle=%#x",
> +		      endpoint_ctx->ep_handle, endpoint_ctx->dev_handle);
> +
> +	if (atomic_read(&ma_dev->unresponsive_client)) {
> +		mausb_pr_err("Client is not responsive anymore - drop endpoint immediately");
> +		endpoint->hcpriv = NULL;
> +		kfree(endpoint_ctx);
> +		return status;

This is an example where disabling GCC's uninitialized variable checking
hurts the code.  This should be "return 0;" or "return -ESOMETHING;".


> +	}
> +
> +	status = mausb_epinactivate_event_to_user(ma_dev,
> +						  usb_device_ctx->dev_handle,
> +						  endpoint_ctx->ep_handle);
> +
> +	mausb_pr_info("epinactivate request ep_handle=%#x, dev_handle=%#x, status=%d",
> +		       endpoint_ctx->ep_handle, endpoint_ctx->dev_handle,
> +		       status);
> +
> +	while (true) {
> +		status = mausb_epdelete_event_to_user(ma_dev,
> +						usb_device_ctx->dev_handle,
> +						endpoint_ctx->ep_handle);
> +
> +		mausb_pr_info("ep_handle_delete_request, ep_handle=%#x, dev_handle=%#x, status=%d",
> +			      endpoint_ctx->ep_handle, endpoint_ctx->dev_handle,
> +			      status);
> +		/* sleep for 10 ms to give device some time */
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Delete this an just put 10ms in the usleep_range() parameters.  Ideally
code should document itself.

> +		if (status == -EBUSY) {
> +			if (++retries < MAUSB_BUSY_RETRIES_COUNT)
> +				usleep_range(MAUSB_BUSY_TIMEOUT_MIN,
> +					MAUSB_BUSY_TIMEOUT_MAX);

Delete the MAUSB_BUSY_TIMEOUT_MIN defines.

> +			else
> +				return status;

return -EBUSY.  There are a bunch of places which "return status;"
that should be updated to "return 0;" etc.

> +		} else {
> +			break;
> +		}
> +	}
> +
> +	mausb_pr_info("Endpoint dropped ep_handle=%#x, dev_handle=%#x",
> +		      endpoint_ctx->ep_handle, endpoint_ctx->dev_handle);
> +
> +	endpoint->hcpriv = NULL;
> +	kfree(endpoint_ctx);
> +	return status;
> +}
> +

[ snip ]

> +	if (unlikely(dev_speed == -EINVAL)) {
> +		mausb_pr_err("bad dev_speed");
> +		return -EINVAL;
> +	}

Remove all the likely()/unlikely().  Those only belong in core kernel,
not in drivers.  Search and delete.

	if (dev_speed < 0)
		return dev_speed;


[ snip ]

> +	if (!usb_device_ctx ||
> +		usb_device_ctx->dev_handle == DEV_HANDLE_NOT_ASSIGNED)
> +		return 0;

Align conditions like this:

	if (!usb_device_ctx ||
	    usb_device_ctx->dev_handle == DEV_HANDLE_NOT_ASSIGNED)
		return 0;

> +#ifdef ISOCH_CALLBACKS
> +int mausb_sec_event_ring_setup(struct usb_hcd *hcd, unsigned int intr_num)
> +{
> +	mausb_pr_debug("");
> +	return 0;
> +}


If possible, let's delete all the dummy functions.


> +	if (mausb_isoch_data_event(event))
> +		if (event->data.direction == MAUSB_DATA_MSG_DIRECTION_IN)
> +			status = mausb_receive_isoch_in_data(dev, event,
> +							     urb_ctx);
> +		else
> +			status = mausb_receive_isoch_out(dev, event, urb_ctx);
> +	else
> +		if (event->data.direction == MAUSB_DATA_MSG_DIRECTION_IN)
> +			status = mausb_receive_in_data(dev, event, urb_ctx);
> +		else
> +			status = mausb_receive_out_data(dev, event, urb_ctx);


Multi-line indent blocks get curly braces for readability.


> +static int mausb_init_header_data_chunk(struct ma_usb_hdr_common *common_hdr,
> +					struct list_head *chunks_list,
> +					uint32_t *num_of_data_chunks)
> +{
> +	int status = mausb_add_data_chunk(common_hdr,
> +					  MAUSB_TRANSFER_HDR_SIZE,
> +					  chunks_list);
> +	/* Success */
> +	if (!status)
> +		++(*num_of_data_chunks);
> +
> +	return status;
> +}

When you need comments to explain the success path it means the code
is messy.  The success path should be at indent level one tab and the
failure path should be indented two tabs.  Always do "failure handling",
not "success handling" like this.

	int status;

	status = mausb_add_data_chunk(common_hdr, MAUSB_TRANSFER_HDR_SIZE,
				      chunks_list);
	if (status)
		return status;

	++(*num_of_data_chunks);

	return 0;

> +static int mausb_init_control_data_chunk(struct mausb_event *event,
> +				  struct list_head *chunks_list,
> +				  uint32_t *num_of_data_chunks)
> +{
> +	int status = 0;
> +
> +	if (!event->data.first_control_packet)
> +		goto l_return;

Just do a direct return.  This do-nothing goto is pointless.  What does
the l in l_return even mean?  It's confusing to the readers because is
this really supposed to be a success path?

	if (!event->data.first_control_packet)
		return 0;

A "return 0;" is clearly intentional and instantly clear.

> +
> +	status = mausb_add_data_chunk(
> +				((struct urb *)event->data.urb)->setup_packet,
> +				MAUSB_CONTROL_SETUP_SIZE, chunks_list);
> +	/* Success */
> +	if (!status)
> +		++(*num_of_data_chunks);
> +
> +l_return:
> +	return status;
> +}

Anyway, run it through checkpatch and resend, then I look at it
properly.

regards,
dan carpenter

Powered by blists - more mailing lists