diff mbox series

PCI: mt7621: Use dev_err_probe()

Message ID	202303231145121987818@zte.com.cn
State	New
Headers	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: Thu, 23 Mar 2023 11:45:12 +0800 (CST) Message-ID: <202303231145121987818@zte.com.cn> Mime-Version: 1.0 From: <ye.xingchen@zte.com.cn> To: <sergio.paracuellos@gmail.com> Cc: <lpieralisi@kernel.org>, <kw@linux.com>, <robh@kernel.org>, <bhelgaas@google.com>, <matthias.bgg@gmail.com>, <angelogioacchino.delregno@collabora.com>, <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, <linux-mediatek@lists.infradead.org> Subject: =?utf-8?q?=5BPATCH=5D_PCI=3A_mt7621=3A_Use_dev=5Ferr=5Fprobe=28=29?= Content-Type: text/plain; charset="UTF-8" Precedence: bulk
Series	PCI: mt7621: Use dev_err_probe() \| PCI: mt7621: Use dev_err_probe()

Commit Message

ye.xingchen@zte.com.cn March 23, 2023, 3:45 a.m. UTC

  From: Ye Xingchen <ye.xingchen@zte.com.cn>

Replace the open-code with dev_err_probe() to simplify the code.

Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
---
 drivers/pci/controller/pcie-mt7621.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Sergio Paracuellos March 23, 2023, 6:23 a.m. UTC | #1

Hi,

On Thu, Mar 23, 2023 at 4:45 AM <ye.xingchen@zte.com.cn> wrote:
>
> From: Ye Xingchen <ye.xingchen@zte.com.cn>
>
> Replace the open-code with dev_err_probe() to simplify the code.
>
> Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> ---
>  drivers/pci/controller/pcie-mt7621.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 63a5f4463a9f..964de0e8c6a0 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -220,10 +220,9 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
>         }
>
>         port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
> -       if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
> -               dev_err(dev, "failed to get pcie%d reset control\n", slot);
> -               return PTR_ERR(port->pcie_rst);
> -       }
> +
> +       return dev_err_probe(dev, PTR_ERR(port->pcie_rst),
> +                            "failed to get pcie%d reset control\n", slot);
>
>         snprintf(name, sizeof(name), "pcie-phy%d", slot);
>         port->phy = devm_of_phy_get(dev, node, name);

So this code is unreachable now. Please fix your tools.

> --
> 2.25.1

Also, this is not a probe(), so I don't see a point of using
dev_err_probe() here.

Thanks,
    Sergio Paracuellos

AngeloGioacchino Del Regno March 23, 2023, 8:21 a.m. UTC | #2

Il 23/03/23 04:45, ye.xingchen@zte.com.cn ha scritto:
> From: Ye Xingchen <ye.xingchen@zte.com.cn>
> 
> Replace the open-code with dev_err_probe() to simplify the code.
> 
> Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> ---
>   drivers/pci/controller/pcie-mt7621.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 63a5f4463a9f..964de0e8c6a0 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -220,10 +220,9 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
>   	}
> 
>   	port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
> -	if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
> -		dev_err(dev, "failed to get pcie%d reset control\n", slot);
> -		return PTR_ERR(port->pcie_rst);
> -	}
> +
> +	return dev_err_probe(dev, PTR_ERR(port->pcie_rst),
> +			     "failed to get pcie%d reset control\n", slot);

That's breaking this function. You're unconditionally returning.

I think that this is fine as it is, but if you really want to use dev_err_probe()
here, this could be...

ret = dev_err_probe(dev, PTR_ERR(port->pcie_rst), "failed ...." ...);
if (ret)
	return ret;

Regards,
Angelo

> 
>   	snprintf(name, sizeof(name), "pcie-phy%d", slot);
>   	port->phy = devm_of_phy_get(dev, node, name);

Dan Carpenter April 3, 2023, 4:41 a.m. UTC | #3

Hi,

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/ye-xingchen-zte-com-cn/PCI-mt7621-Use-dev_err_probe/20230323-114623
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/202303231145121987818%40zte.com.cn
patch subject: [PATCH] PCI: mt7621: Use dev_err_probe()
config: s390-randconfig-m031-20230329 (https://download.01.org/0day-ci/archive/20230401/202304010325.2OPFvIm3-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202304010325.2OPFvIm3-lkp@intel.com/

smatch warnings:
drivers/pci/controller/pcie-mt7621.c:224 mt7621_pcie_parse_port() warn: passing zero to 'PTR_ERR'
drivers/pci/controller/pcie-mt7621.c:227 mt7621_pcie_parse_port() warn: ignoring unreachable code.

vim +/PTR_ERR +224 drivers/pci/controller/pcie-mt7621.c

ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  198  static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  199  				  struct device_node *node,
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  200  				  int slot)
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  201  {
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  202  	struct mt7621_pcie_port *port;
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  203  	struct device *dev = pcie->dev;
fab6710e4c51f4 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-04-13  204  	struct platform_device *pdev = to_platform_device(dev);
61f9bde6ea578f drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2019-01-04  205  	char name[10];
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  206  	int err;
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  207  
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  208  	port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  209  	if (!port)
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  210  		return -ENOMEM;
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  211  
108b2f2a972454 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-11-23  212  	port->base = devm_platform_ioremap_resource(pdev, slot + 1);
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  213  	if (IS_ERR(port->base))
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  214  		return PTR_ERR(port->base);
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  215  
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  216  	port->clk = devm_get_clk_from_child(dev, node, NULL);
cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  217  	if (IS_ERR(port->clk)) {
cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  218  		dev_err(dev, "failed to get pcie%d clock\n", slot);
cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  219  		return PTR_ERR(port->clk);
cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  220  	}
cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  221  
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  222  	port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
9873bac812f262 drivers/pci/controller/pcie-mt7621.c    Ye Xingchen        2023-03-23  223  
9873bac812f262 drivers/pci/controller/pcie-mt7621.c    Ye Xingchen        2023-03-23 @224  	return dev_err_probe(dev, PTR_ERR(port->pcie_rst),
                                                                                                                          ^^^^^^^^^^^^^^^^^^^^^^^

9873bac812f262 drivers/pci/controller/pcie-mt7621.c    Ye Xingchen        2023-03-23  225  			     "failed to get pcie%d reset control\n", slot);
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  226  
61f9bde6ea578f drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2019-01-04 @227  	snprintf(name, sizeof(name), "pcie-phy%d", slot);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  228  	port->phy = devm_of_phy_get(dev, node, name);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  229  	if (IS_ERR(port->phy)) {
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  230  		dev_err(dev, "failed to get pcie-phy%d\n", slot);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  231  		err = PTR_ERR(port->phy);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  232  		goto remove_reset;
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  233  	}
61f9bde6ea578f drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2019-01-04  234  
b27e35f91c75cf drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-13  235  	port->gpio_rst = devm_gpiod_get_index_optional(dev, "reset", slot,
b27e35f91c75cf drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-13  236  						       GPIOD_OUT_LOW);
825c6f470c62da drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-20  237  	if (IS_ERR(port->gpio_rst)) {
2bdd5238e756aa drivers/pci/controller/pcie-mt7621.c    Sergio Paracuellos 2021-09-22  238  		dev_err(dev, "failed to get GPIO for PCIe%d\n", slot);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  239  		err = PTR_ERR(port->gpio_rst);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  240  		goto remove_reset;
825c6f470c62da drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-20  241  	}
b27e35f91c75cf drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-13  242  
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  243  	port->slot = slot;
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  244  	port->pcie = pcie;
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  245  
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  246  	INIT_LIST_HEAD(&port->list);
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  247  	list_add_tail(&port->list, &pcie->ports);
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  248  
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  249  	return 0;
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  250  
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  251  remove_reset:
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  252  	reset_control_put(port->pcie_rst);
2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  253  	return err;
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  254  }
ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  255

Dan Carpenter April 3, 2023, 4:50 a.m. UTC | #4

On Thu, Mar 23, 2023 at 07:23:26AM +0100, Sergio Paracuellos wrote:
> Hi,
> 
> On Thu, Mar 23, 2023 at 4:45 AM <ye.xingchen@zte.com.cn> wrote:
> >
> > From: Ye Xingchen <ye.xingchen@zte.com.cn>
> >
> > Replace the open-code with dev_err_probe() to simplify the code.
> >
> > Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> > ---
> >  drivers/pci/controller/pcie-mt7621.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> > index 63a5f4463a9f..964de0e8c6a0 100644
> > --- a/drivers/pci/controller/pcie-mt7621.c
> > +++ b/drivers/pci/controller/pcie-mt7621.c
> > @@ -220,10 +220,9 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> >         }
> >
> >         port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
> > -       if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {

So the theory here is that -EPROBE_DEFER is recoverable but other errors
are not so we just ignore them?  Error pointers will trigger a WARN() in
mt7621_control_assert/deassert().

> > --
> > 2.25.1
> 
> Also, this is not a probe(), so I don't see a point of using
> dev_err_probe() here.

It's a weird thing to return -EPROBE_DEFER from something which is not
a probe function...  Someone told me I should write a Smatch check for
it but I never got around to doing that.

In this case, I guess the user is supposed to see the error message and
manually fix the probe order?  The dev_err_probe() will change the error
message into a debug message so the user will not see it and will not be
able to fix it.  So using dev_err_probe() will break things for the
user.

regards,
dan carpenter

Sergio Paracuellos April 3, 2023, 5:05 a.m. UTC | #5

On Mon, Apr 3, 2023 at 6:41 AM Dan Carpenter <error27@gmail.com> wrote:
>
> Hi,
>
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/ye-xingchen-zte-com-cn/PCI-mt7621-Use-dev_err_probe/20230323-114623
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> patch link:    https://lore.kernel.org/r/202303231145121987818%40zte.com.cn

So, I already replied to this proposed patch clearly saying that this
makes the rest of the code unreachable, so it is a clear NAK.
Why is this applied to the intel-lab-lkp tree? Just to be able to test
the changes?

Thanks,
    Sergio Paracuellos

> patch subject: [PATCH] PCI: mt7621: Use dev_err_probe()
> config: s390-randconfig-m031-20230329 (https://download.01.org/0day-ci/archive/20230401/202304010325.2OPFvIm3-lkp@intel.com/config)
> compiler: s390-linux-gcc (GCC) 12.1.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Link: https://lore.kernel.org/r/202304010325.2OPFvIm3-lkp@intel.com/
>
> smatch warnings:
> drivers/pci/controller/pcie-mt7621.c:224 mt7621_pcie_parse_port() warn: passing zero to 'PTR_ERR'
> drivers/pci/controller/pcie-mt7621.c:227 mt7621_pcie_parse_port() warn: ignoring unreachable code.
>
> vim +/PTR_ERR +224 drivers/pci/controller/pcie-mt7621.c
>
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  198  static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  199                                 struct device_node *node,
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  200                                 int slot)
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  201  {
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  202       struct mt7621_pcie_port *port;
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  203       struct device *dev = pcie->dev;
> fab6710e4c51f4 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-04-13  204       struct platform_device *pdev = to_platform_device(dev);
> 61f9bde6ea578f drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2019-01-04  205       char name[10];
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  206       int err;
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  207
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  208       port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  209       if (!port)
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  210               return -ENOMEM;
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  211
> 108b2f2a972454 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-11-23  212       port->base = devm_platform_ioremap_resource(pdev, slot + 1);
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  213       if (IS_ERR(port->base))
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  214               return PTR_ERR(port->base);
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  215
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  216       port->clk = devm_get_clk_from_child(dev, node, NULL);
> cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  217       if (IS_ERR(port->clk)) {
> cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  218               dev_err(dev, "failed to get pcie%d clock\n", slot);
> cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  219               return PTR_ERR(port->clk);
> cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  220       }
> cc4e864a5ce4c1 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-05-05  221
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  222       port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
> 9873bac812f262 drivers/pci/controller/pcie-mt7621.c    Ye Xingchen        2023-03-23  223
> 9873bac812f262 drivers/pci/controller/pcie-mt7621.c    Ye Xingchen        2023-03-23 @224       return dev_err_probe(dev, PTR_ERR(port->pcie_rst),
>                                                                                                                           ^^^^^^^^^^^^^^^^^^^^^^^
>
> 9873bac812f262 drivers/pci/controller/pcie-mt7621.c    Ye Xingchen        2023-03-23  225                            "failed to get pcie%d reset control\n", slot);
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  226
> 61f9bde6ea578f drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2019-01-04 @227       snprintf(name, sizeof(name), "pcie-phy%d", slot);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  228       port->phy = devm_of_phy_get(dev, node, name);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  229       if (IS_ERR(port->phy)) {
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  230               dev_err(dev, "failed to get pcie-phy%d\n", slot);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  231               err = PTR_ERR(port->phy);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  232               goto remove_reset;
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  233       }
> 61f9bde6ea578f drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2019-01-04  234
> b27e35f91c75cf drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-13  235       port->gpio_rst = devm_gpiod_get_index_optional(dev, "reset", slot,
> b27e35f91c75cf drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-13  236                                                      GPIOD_OUT_LOW);
> 825c6f470c62da drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-20  237       if (IS_ERR(port->gpio_rst)) {
> 2bdd5238e756aa drivers/pci/controller/pcie-mt7621.c    Sergio Paracuellos 2021-09-22  238               dev_err(dev, "failed to get GPIO for PCIe%d\n", slot);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  239               err = PTR_ERR(port->gpio_rst);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  240               goto remove_reset;
> 825c6f470c62da drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-20  241       }
> b27e35f91c75cf drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2020-03-13  242
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  243       port->slot = slot;
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  244       port->pcie = pcie;
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  245
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  246       INIT_LIST_HEAD(&port->list);
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  247       list_add_tail(&port->list, &pcie->ports);
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  248
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  249       return 0;
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  250
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  251  remove_reset:
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  252       reset_control_put(port->pcie_rst);
> 2d3d288f0eaf10 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2021-06-07  253       return err;
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  254  }
> ad9c87e129d139 drivers/staging/mt7621-pci/pci-mt7621.c Sergio Paracuellos 2018-11-04  255
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
>

Dan Carpenter April 3, 2023, 6:09 a.m. UTC | #6

On Mon, Apr 03, 2023 at 07:05:56AM +0200, Sergio Paracuellos wrote:
> On Mon, Apr 3, 2023 at 6:41 AM Dan Carpenter <error27@gmail.com> wrote:
> >
> > Hi,
> >
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/ye-xingchen-zte-com-cn/PCI-mt7621-Use-dev_err_probe/20230323-114623
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> > patch link:    https://lore.kernel.org/r/202303231145121987818%40zte.com.cn
> 
> So, I already replied to this proposed patch clearly saying that this
> makes the rest of the code unreachable, so it is a clear NAK.
> Why is this applied to the intel-lab-lkp tree? Just to be able to test
> the changes?
> 

These emails are automatically generated by kbuild-bot.  I don't know
how kbuild-bot internals work.  I just review some of the Smatch related
warnings and hit forward or ignore them.

Normally, I don't look at the context outside of the email but to be
honest, I was curious enough about this one that I looked it up on the
list.  I knew it was NAKed but I set the email anyway hoping that maybe
people would see the extra Smatch warning and be encouraged to run
Smatch on their code in the future to avoid potential embarrassment.

regards,
dan carpenter

Sergio Paracuellos April 3, 2023, 7:12 a.m. UTC | #7

On Mon, Apr 3, 2023 at 6:50 AM Dan Carpenter <error27@gmail.com> wrote:
>
> On Thu, Mar 23, 2023 at 07:23:26AM +0100, Sergio Paracuellos wrote:
> > Hi,
> >
> > On Thu, Mar 23, 2023 at 4:45 AM <ye.xingchen@zte.com.cn> wrote:
> > >
> > > From: Ye Xingchen <ye.xingchen@zte.com.cn>
> > >
> > > Replace the open-code with dev_err_probe() to simplify the code.
> > >
> > > Signed-off-by: Ye Xingchen <ye.xingchen@zte.com.cn>
> > > ---
> > >  drivers/pci/controller/pcie-mt7621.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> > > index 63a5f4463a9f..964de0e8c6a0 100644
> > > --- a/drivers/pci/controller/pcie-mt7621.c
> > > +++ b/drivers/pci/controller/pcie-mt7621.c
> > > @@ -220,10 +220,9 @@ static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
> > >         }
> > >
> > >         port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
> > > -       if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
>
> So the theory here is that -EPROBE_DEFER is recoverable but other errors
> are not so we just ignore them?  Error pointers will trigger a WARN() in
> mt7621_control_assert/deassert().
>
>
> > > --
> > > 2.25.1
> >
> > Also, this is not a probe(), so I don't see a point of using
> > dev_err_probe() here.
>
> It's a weird thing to return -EPROBE_DEFER from something which is not
> a probe function...  Someone told me I should write a Smatch check for
> it but I never got around to doing that.

I don't remember clearly why this was returned in the first instance.
I think I just took the idea from pcie-mediatek driver for arm64 SoCs
platforms here:

https://elixir.bootlin.com/linux/v6.3-rc5/source/drivers/pci/controller/pcie-mediatek.c#L967

Thanks,
    Sergio Paracuellos
>
> In this case, I guess the user is supposed to see the error message and
> manually fix the probe order?  The dev_err_probe() will change the error
> message into a debug message so the user will not see it and will not be
> able to fix it.  So using dev_err_probe() will break things for the
> user.
>
> regards,
> dan carpenter
>

Sergio Paracuellos April 3, 2023, 7:12 a.m. UTC | #8

On Mon, Apr 3, 2023 at 8:11 AM Dan Carpenter <error27@gmail.com> wrote:
>
> On Mon, Apr 03, 2023 at 07:05:56AM +0200, Sergio Paracuellos wrote:
> > On Mon, Apr 3, 2023 at 6:41 AM Dan Carpenter <error27@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > >
> > > url:    https://github.com/intel-lab-lkp/linux/commits/ye-xingchen-zte-com-cn/PCI-mt7621-Use-dev_err_probe/20230323-114623
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
> > > patch link:    https://lore.kernel.org/r/202303231145121987818%40zte.com.cn
> >
> > So, I already replied to this proposed patch clearly saying that this
> > makes the rest of the code unreachable, so it is a clear NAK.
> > Why is this applied to the intel-lab-lkp tree? Just to be able to test
> > the changes?
> >
>
> These emails are automatically generated by kbuild-bot.  I don't know
> how kbuild-bot internals work.  I just review some of the Smatch related
> warnings and hit forward or ignore them.
>
> Normally, I don't look at the context outside of the email but to be
> honest, I was curious enough about this one that I looked it up on the
> list.  I knew it was NAKed but I set the email anyway hoping that maybe
> people would see the extra Smatch warning and be encouraged to run
> Smatch on their code in the future to avoid potential embarrassment.

I see :). Thanks for the explanation, Dan.

Best regards,
    Sergio Paracuellos
>
> regards,
> dan carpenter
>
>

diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 63a5f4463a9f..964de0e8c6a0 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -220,10 +220,9 @@  static int mt7621_pcie_parse_port(struct mt7621_pcie *pcie,
 	}

 	port->pcie_rst = of_reset_control_get_exclusive(node, NULL);
-	if (PTR_ERR(port->pcie_rst) == -EPROBE_DEFER) {
-		dev_err(dev, "failed to get pcie%d reset control\n", slot);
-		return PTR_ERR(port->pcie_rst);
-	}
+
+	return dev_err_probe(dev, PTR_ERR(port->pcie_rst),
+			     "failed to get pcie%d reset control\n", slot);

 	snprintf(name, sizeof(name), "pcie-phy%d", slot);
 	port->phy = devm_of_phy_get(dev, node, name);