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
