| Message ID | 20240830170014.15389-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: alacritech: Partially revert "net: alacritech: Switch to use dev_err_probe()" | expand |
On Fri, Aug 30, 2024 at 07:00:14PM +0200, Krzysztof Kozlowski wrote: > This reverts commit bf4d87f884fe8a4b6b61fe4d0e05f293d08df61c because it > introduced dev_err_probe() in non-probe path, which is not desired. > Calling it after successful probe, dev_err_probe() will set deferred > status on the device already probed. See also documentation of > dev_err_probe(). I agree that using dev_err_probe() outside of a probe path is inappropriate. And I agree that your patch addresses that problem in the context of changes made by the cited commit. But, based on my reading of dev_err_probe(), I think the text above is slightly misleading. This is because deferred status is only set in the case where the err passed to dev_err_probe() is -EPROBE_DEFER. And I do suspect that is never the case for the calls removed by this patch. > Fixes: bf4d87f884fe ("net: alacritech: Switch to use dev_err_probe()") > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> ...
On Fri, Aug 30, 2024 at 07:28:44PM +0100, Simon Horman wrote: > On Fri, Aug 30, 2024 at 07:00:14PM +0200, Krzysztof Kozlowski wrote: > > This reverts commit bf4d87f884fe8a4b6b61fe4d0e05f293d08df61c because it > > introduced dev_err_probe() in non-probe path, which is not desired. > > Calling it after successful probe, dev_err_probe() will set deferred > > status on the device already probed. See also documentation of > > dev_err_probe(). > > I agree that using dev_err_probe() outside of a probe path is > inappropriate. And I agree that your patch addresses that problem > in the context of changes made by the cited commit. Maybe device_set_deferred_probe_reason() could call device_is_bound() is check the device is not actually bound, and hence still in probe, and do a dev_warn(). That should help catch these errors. I assume the developers submitting these patches are also using a bot. It would be good if the bot could be trained to follow the call paths and ensure it only reports cases which are probe. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Friday, August 30, 2024 1:34 PM > To: Simon Horman <horms@kernel.org> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; Lino Sanfilippo > <LinoSanfilippo@gmx.de>; David S. Miller <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Yang Ruibin > <11162571@vivo.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] net: alacritech: Partially revert "net: alacritech: Switch to use > dev_err_probe()" > > On Fri, Aug 30, 2024 at 07:28:44PM +0100, Simon Horman wrote: > > On Fri, Aug 30, 2024 at 07:00:14PM +0200, Krzysztof Kozlowski wrote: > > > This reverts commit bf4d87f884fe8a4b6b61fe4d0e05f293d08df61c because it > > > introduced dev_err_probe() in non-probe path, which is not desired. > > > Calling it after successful probe, dev_err_probe() will set deferred > > > status on the device already probed. See also documentation of > > > dev_err_probe(). > > > > I agree that using dev_err_probe() outside of a probe path is > > inappropriate. And I agree that your patch addresses that problem > > in the context of changes made by the cited commit. > > Maybe device_set_deferred_probe_reason() could call device_is_bound() > is check the device is not actually bound, and hence still in probe, > and do a dev_warn(). That should help catch these errors. > That seems reasonable to me. > I assume the developers submitting these patches are also using a > bot. It would be good if the bot could be trained to follow the call > paths and ensure it only reports cases which are probe. > > Andrew
On 30/08/2024 20:28, Simon Horman wrote: > On Fri, Aug 30, 2024 at 07:00:14PM +0200, Krzysztof Kozlowski wrote: >> This reverts commit bf4d87f884fe8a4b6b61fe4d0e05f293d08df61c because it >> introduced dev_err_probe() in non-probe path, which is not desired. >> Calling it after successful probe, dev_err_probe() will set deferred >> status on the device already probed. See also documentation of >> dev_err_probe(). > > I agree that using dev_err_probe() outside of a probe path is > inappropriate. And I agree that your patch addresses that problem > in the context of changes made by the cited commit. > > But, based on my reading of dev_err_probe(), I think the text above is > slightly misleading. This is because deferred status is only set in the > case where the err passed to dev_err_probe() is -EPROBE_DEFER. And I do That's true and indeed request_firmware() will not return EPROBE_DEFER. I'll update commit msg. Best regards, Krzysztof
diff --git a/drivers/net/ethernet/alacritech/slicoss.c b/drivers/net/ethernet/alacritech/slicoss.c index 7f0c07c20be3..f62851708d4f 100644 --- a/drivers/net/ethernet/alacritech/slicoss.c +++ b/drivers/net/ethernet/alacritech/slicoss.c @@ -1051,9 +1051,11 @@ static int slic_load_rcvseq_firmware(struct slic_device *sdev) file = (sdev->model == SLIC_MODEL_OASIS) ? SLIC_RCV_FIRMWARE_OASIS : SLIC_RCV_FIRMWARE_MOJAVE; err = request_firmware(&fw, file, &sdev->pdev->dev); - if (err) - return dev_err_probe(&sdev->pdev->dev, err, + if (err) { + dev_err(&sdev->pdev->dev, "failed to load receive sequencer firmware %s\n", file); + return err; + } /* Do an initial sanity check concerning firmware size now. A further * check follows below. */ @@ -1124,9 +1126,10 @@ static int slic_load_firmware(struct slic_device *sdev) file = (sdev->model == SLIC_MODEL_OASIS) ? SLIC_FIRMWARE_OASIS : SLIC_FIRMWARE_MOJAVE; err = request_firmware(&fw, file, &sdev->pdev->dev); - if (err) - return dev_err_probe(&sdev->pdev->dev, err, - "failed to load firmware %s\n", file); + if (err) { + dev_err(&sdev->pdev->dev, "failed to load firmware %s\n", file); + return err; + } /* Do an initial sanity check concerning firmware size now. A further * check follows below. */
This reverts commit bf4d87f884fe8a4b6b61fe4d0e05f293d08df61c because it introduced dev_err_probe() in non-probe path, which is not desired. Calling it after successful probe, dev_err_probe() will set deferred status on the device already probed. See also documentation of dev_err_probe(). Fixes: bf4d87f884fe ("net: alacritech: Switch to use dev_err_probe()") Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- drivers/net/ethernet/alacritech/slicoss.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)