Date:   Thu, 2 Feb 2023 16:35:40 +0100
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Marc Kleine-Budde <mkl@pengutronix.de>
Cc:     Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
        linux-can@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH] can: etas_es58x: do not send disable channel command if
 device is unplugged
Message-ID: <Y9vYTPURT4hDSH4n@kroah.com>
References: <20230128133815.1796221-1-mailhol.vincent@wanadoo.fr>
 <20230202151622.sotqfwmgwwtgv4dl@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230202151622.sotqfwmgwwtgv4dl@pengutronix.de>
Precedence: bulk

On Thu, Feb 02, 2023 at 04:16:22PM +0100, Marc Kleine-Budde wrote:
> On 28.01.2023 22:38:15, Vincent Mailhol wrote:
> > When turning the network interface down, es58x_stop() is called and
> > will send a command to the ES58x device to disable the channel
> > c.f. es58x_ops::disable_channel().
> > 
> > However, if the device gets unplugged while the network interface is
> > still up, es58x_ops::disable_channel() will obviously fail to send the
> > URB command and the driver emits below error message:
> > 
> >   es58x_submit_urb: USB send urb failure: -ENODEV
> > 
> > Check the usb device state before sending the disable channel command
> > in order to silence above error message.
> > 
> > Update the documentation of es58x_stop() accordingly.
> > 
> > The check being added in es58x_stop() is:
> > 
> >   	if (es58x_dev->udev->state >= USB_STATE_UNAUTHENTICATED)
> > 
> > This is just the negation of the check done in usb_submit_urb()[1].
> > 
> > [1] usb_submit_urb(), verify usb device's state.
> > Link: https://elixir.bootlin.com/linux/v6.1/source/drivers/usb/core/urb.c#L384
> > 
> > Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > As far as I know, there doesn't seem to be an helper function to check
> > udev->state values. If anyone is aware of such helper function, let me
> > know..
> 
> The constant USB_STATE_UNAUTHENTICATED is not used very often in the
> kernel. Maybe Greg can point out what is best to do here.

Don't do anything in the driver here, except maybe turn off that useless
"error message" in the urb submission function?  A USB driver shouldn't
be poking around in the state of the device at all, as it's about to go
away if it's been physically removed.

The urb submission will fail if the device is removed, handle the error
and move on, that's a totally normal situation as you say it happens a
lot :)

Don't spam error logs for common things please.

thanks,

greg k-h