Re: [PATCH 2/2] staging: gpib: Agilent usb code cleanup

[Dan Carpenter]

This patch does too many things...  It should be split up.  People
complain about this requirement a lot, but eventually it will become
instinctive.  I use `git citool` so I can highlight and click to add
lines to a commit.  In this code there were some dev_info() changes
mixed into the unwind code in ->attach() that were hard to separate out
into their own commit but it wasn't too complicated.

On Sat, Jan 18, 2025 at 03:50:46PM +0100, Dave Penkler wrote:
> Remove useless #ifdef RESET_USB_CONFIG code.
> 

patch 1.

> Change kalloc / memset to kzalloc
> 

patch 2.

> The attach function was not freeing the private data on error
> returns. Separate the releasing of urbs and private data and
> add a common error exit for attach failure.
> 
> Set the board private data pointer to NULL after freeing
> the private data.

By setting the private data, this patch actually does fix the
double free that I mentioned earlier.  It changes the ->detach into
a no-op if ->attach fails.  Needs a Fixes tag.  ;)

But I still hope my blog will convince you that the error handling can be
re-written in a better way.  It shouldn't matter if ->private_data is
NULL or non-NULL because the caller should only have to handle success
or failure.  The caller shouldn't have to handle a dozen different
failure modes:

1) Failure but the ->private_data is NULL
2) Failure but the foo->frob pointer is an error pointer
3) Failure but the foo->frob pointer needs to be freed.
4) Failure but the foo->frob pointer contains other pointers which
   need to be freed.
5) ...

It should just be

1) Success: Everything is allocated
2) Failure: Everything is cleaned up and any accesses are probably a
   use after free.

> 
> Reduce console spam by emitting only one attach message.
> 
> Change last pr_err in attach to dev_err
> 

These last two can probably be combined into one patch?

> @@ -1388,11 +1367,19 @@ static int agilent_82357a_attach(gpib_board_t *board, const gpib_board_config_t
>  	retval = agilent_82357a_init(board);
>  
>  	if (retval < 0)	{
> -		mutex_unlock(&agilent_82357a_hotplug_lock);
> -		return retval;
> +		agilent_82357a_cleanup_urbs(a_priv);
> +		agilent_82357a_release_urbs(a_priv);
> +		goto attach_fail;
>  	}

In my blog talk about how every allocation function should have a
matching free() function.  These two functions match
agilent_82357a_setup_urbs() so we should have a single function to
release the urbs.

regards,
dan carpenter