| Message ID | 20230828121235.13953-1-adiupina@astralinux.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] fsl_ucc_hdlc: process the result of hold_open() | expand |
Le 28/08/2023 à 14:12, Alexandra Diupina a écrit : > [Vous ne recevez pas souvent de courriers de adiupina@astralinux.ru. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > Process the result of hold_open() and return it from > uhdlc_open() in case of an error > It is necessary to pass the error code up the control flow, > similar to a possible error in request_irq() > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC") > Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > v3: Fix the commits tree > v2: Remove the 'rc' variable (stores the return value of the > hdlc_open()) as Christophe Leroy <christophe.leroy@csgroup.eu> suggested > drivers/net/wan/fsl_ucc_hdlc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c > index 47c2ad7a3e42..4164abea7725 100644 > --- a/drivers/net/wan/fsl_ucc_hdlc.c > +++ b/drivers/net/wan/fsl_ucc_hdlc.c > @@ -731,7 +731,7 @@ static int uhdlc_open(struct net_device *dev) > napi_enable(&priv->napi); > netdev_reset_queue(dev); > netif_start_queue(dev); > - hdlc_open(dev); > + return hdlc_open(dev); > } > > return 0; > -- > 2.30.2 >
On Mon, 28 Aug 2023 15:12:35 +0300 Alexandra Diupina wrote: > Process the result of hold_open() and return it from > uhdlc_open() in case of an error > It is necessary to pass the error code up the control flow, > similar to a possible error in request_irq() > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC") > Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> > --- > v3: Fix the commits tree > v2: Remove the 'rc' variable (stores the return value of the > hdlc_open()) as Christophe Leroy <christophe.leroy@csgroup.eu> suggested > drivers/net/wan/fsl_ucc_hdlc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c > index 47c2ad7a3e42..4164abea7725 100644 > --- a/drivers/net/wan/fsl_ucc_hdlc.c > +++ b/drivers/net/wan/fsl_ucc_hdlc.c > @@ -731,7 +731,7 @@ static int uhdlc_open(struct net_device *dev) > napi_enable(&priv->napi); > netdev_reset_queue(dev); > netif_start_queue(dev); > - hdlc_open(dev); > + return hdlc_open(dev); Don't you have to undo all the things done prior to hdlc_open()? Before you post v4 please make sure that you've read: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#resending-after-review Zhao, please review the next version.
Thanks for the review!
28.08.2023 22:38, Jakub Kicinski пишет:
> Don't you have to undo all the things done prior to hdlc_open()?
Yes, it looks like I really need to undo everything that was done before
hdlc_open().
But the question arose - would it be enough to call the uhdlc_close(dev)
function?
In addition, it seems to me and my colleagues that a call to
hdlc_close(dev) should be added to the uhdlc_close() function, similar
to the following functions:
1. drivers/net/wan/n2.c (n2_close function)
2. drivers/net/wan/pc300too.c (pc300_close function)
3. drivers/net/wan/pci200syn.c (pci200_close function)
4. drivers/net/wan/wanxl.c (wanxl_close function)
Tell me, please, are we wrong?
On 9/1/23 13:48, Александра Дюпина wrote: > Thanks for the review! > > 28.08.2023 22:38, Jakub Kicinski пишет: >> Don't you have to undo all the things done prior to hdlc_open()? > Yes, it looks like I really need to undo everything that was done before hdlc_open(). > But the question arose - would it be enough to call the uhdlc_close(dev) function? looks like it is. > In addition, it seems to me and my colleagues that a call to hdlc_close(dev) should be added to the uhdlc_close() function, similar to the yes, take a look at the comment for hdlc_close() following functions: > 1. drivers/net/wan/n2.c (n2_close function) > 2. drivers/net/wan/pc300too.c (pc300_close function) > 3. drivers/net/wan/pci200syn.c (pci200_close function) > 4. drivers/net/wan/wanxl.c (wanxl_close function) > Tell me, please, are we wrong? >
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c index 47c2ad7a3e42..4164abea7725 100644 --- a/drivers/net/wan/fsl_ucc_hdlc.c +++ b/drivers/net/wan/fsl_ucc_hdlc.c @@ -731,7 +731,7 @@ static int uhdlc_open(struct net_device *dev) napi_enable(&priv->napi); netdev_reset_queue(dev); netif_start_queue(dev); - hdlc_open(dev); + return hdlc_open(dev); } return 0;
Process the result of hold_open() and return it from uhdlc_open() in case of an error It is necessary to pass the error code up the control flow, similar to a possible error in request_irq() Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC") Signed-off-by: Alexandra Diupina <adiupina@astralinux.ru> --- v3: Fix the commits tree v2: Remove the 'rc' variable (stores the return value of the hdlc_open()) as Christophe Leroy <christophe.leroy@csgroup.eu> suggested drivers/net/wan/fsl_ucc_hdlc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)