Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 26 Nov 2022 17:51:37 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc:     linux-can@vger.kernel.org, Marc Kleine-Budde <mkl@pengutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        netdev@vger.kernel.org, linux-usb@vger.kernel.org,
        Saeed Mahameed <saeed@kernel.org>,
        Jiri Pirko <jiri@nvidia.com>,
        Lukas Magel <lukas.magel@posteo.net>
Subject: Re: [PATCH v4 2/6] can: etas_es58x: add devlink support
Message-ID: <Y4JEGYMtIWX9clxo@lunn.ch>
References: <20221104073659.414147-1-mailhol.vincent@wanadoo.fr>
 <20221126162211.93322-1-mailhol.vincent@wanadoo.fr>
 <20221126162211.93322-3-mailhol.vincent@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221126162211.93322-3-mailhol.vincent@wanadoo.fr>
Precedence: bulk

> @@ -2196,11 +2198,12 @@ static struct es58x_device *es58x_init_es58x_dev(struct usb_interface *intf,
>  		ops = &es581_4_ops;
>  	}
>  
> -	es58x_dev = devm_kzalloc(dev, es58x_sizeof_es58x_device(param),
> -				 GFP_KERNEL);
> -	if (!es58x_dev)
> +	devlink = devlink_alloc(&es58x_dl_ops, es58x_sizeof_es58x_device(param),
> +				dev);
> +	if (!devlink)
>  		return ERR_PTR(-ENOMEM);
>  
> +	es58x_dev = devlink_priv(devlink);

That is 'interesting'. Makes me wonder about lifetimes of different
objects. Previously your es58x_dev structure would disappear when the
driver is released, or an explicit call to devm_kfree(). Now it
disappears when devlink_free() is called. Any danger of use after free
here?

USB devices always make me wonder about life times rules since they
are probably the mode dynamic sort of device the kernel has the
handle, them just abruptly disappearing.

>  	es58x_dev->param = param;
>  	es58x_dev->ops = ops;
>  	es58x_dev->dev = dev;
> @@ -2247,6 +2250,8 @@ static int es58x_probe(struct usb_interface *intf,
>  	if (ret)
>  		return ret;
>  
> +	devlink_register(priv_to_devlink(es58x_dev));
> +
>  	for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
>  		ret = es58x_init_netdev(es58x_dev, ch_idx);
>  		if (ret) {
> @@ -2272,8 +2277,10 @@ static void es58x_disconnect(struct usb_interface *intf)
>  	dev_info(&intf->dev, "Disconnecting %s %s\n",
>  		 es58x_dev->udev->manufacturer, es58x_dev->udev->product);
>  
> +	devlink_unregister(priv_to_devlink(es58x_dev));
>  	es58x_free_netdevs(es58x_dev);
>  	es58x_free_urbs(es58x_dev);
> +	devlink_free(priv_to_devlink(es58x_dev));
>  	usb_set_intfdata(intf, NULL);

Should devlink_free() be after usb_set_inftdata()?

       Andrew