LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro
@ 2021-08-13 12:27 Andy Shevchenko
  2021-08-13 12:27 ` [PATCH v1 net-next 2/3] ptp_ocp: Convert to use managed functions pcim_* and devm_* Andy Shevchenko
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-13 12:27 UTC (permalink / raw)
  To: Jonathan Lemon, Andy Shevchenko, netdev, linux-kernel; +Cc: Richard Cochran

Eliminate some boilerplate code by using module_pci_driver() instead of
init/exit, and, if needed, moving the salient bits from init into probe.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/ptp/ptp_ocp.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 0d1034e3ed0f..874ea7930079 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -4,7 +4,6 @@
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/ptp_clock_kernel.h>
 
@@ -377,24 +376,7 @@ static struct pci_driver ptp_ocp_driver = {
 	.probe		= ptp_ocp_probe,
 	.remove		= ptp_ocp_remove,
 };
-
-static int __init
-ptp_ocp_init(void)
-{
-	int err;
-
-	err = pci_register_driver(&ptp_ocp_driver);
-	return err;
-}
-
-static void __exit
-ptp_ocp_fini(void)
-{
-	pci_unregister_driver(&ptp_ocp_driver);
-}
-
-module_init(ptp_ocp_init);
-module_exit(ptp_ocp_fini);
+module_pci_driver(ptp_ocp_driver);
 
 MODULE_DESCRIPTION("OpenCompute TimeCard driver");
 MODULE_LICENSE("GPL v2");
-- 
2.30.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1 net-next 2/3] ptp_ocp: Convert to use managed functions pcim_* and devm_*
  2021-08-13 12:27 [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Andy Shevchenko
@ 2021-08-13 12:27 ` Andy Shevchenko
  2021-08-13 12:27 ` [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks Andy Shevchenko
  2021-08-13 18:14 ` [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Jakub Kicinski
  2 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-13 12:27 UTC (permalink / raw)
  To: Jonathan Lemon, Andy Shevchenko, netdev, linux-kernel; +Cc: Richard Cochran

This makes the error handling much more simpler than open-coding everything
and in addition makes the probe function smaller an tidier.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/ptp/ptp_ocp.c | 40 +++++++++-------------------------------
 1 file changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 874ea7930079..5d0c0c0734f2 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -301,30 +301,25 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct ptp_ocp *bp;
 	int err;
 
-	bp = kzalloc(sizeof(*bp), GFP_KERNEL);
+	bp = devm_kzalloc(&pdev->dev, sizeof(*bp), GFP_KERNEL);
 	if (!bp)
 		return -ENOMEM;
 	bp->pdev = pdev;
 	pci_set_drvdata(pdev, bp);
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 	if (err) {
 		dev_err(&pdev->dev, "pci_enable_device\n");
-		goto out_free;
+		return err;
 	}
 
-	err = pci_request_regions(pdev, KBUILD_MODNAME);
+	err = pcim_iomap_regions(pdev, BIT(0), KBUILD_MODNAME);
 	if (err) {
-		dev_err(&pdev->dev, "pci_request_region\n");
-		goto out_disable;
-	}
-
-	bp->base = pci_ioremap_bar(pdev, 0);
-	if (!bp->base) {
 		dev_err(&pdev->dev, "io_remap bar0\n");
-		err = -ENOMEM;
-		goto out_release_regions;
+		return err;
 	}
+
+	bp->base = pcim_iomap_table(pdev)[0];
 	bp->reg = bp->base + OCP_REGISTER_OFFSET;
 	bp->tod = bp->base + TOD_REGISTER_OFFSET;
 	bp->ptp_info = ptp_ocp_clock_info;
@@ -332,29 +327,17 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	err = ptp_ocp_check_clock(bp);
 	if (err)
-		goto out;
+		return err;
 
 	bp->ptp = ptp_clock_register(&bp->ptp_info, &pdev->dev);
 	if (IS_ERR(bp->ptp)) {
 		dev_err(&pdev->dev, "ptp_clock_register\n");
-		err = PTR_ERR(bp->ptp);
-		goto out;
+		return PTR_ERR(bp->ptp);
 	}
 
 	ptp_ocp_info(bp);
 
 	return 0;
-
-out:
-	pci_iounmap(pdev, bp->base);
-out_release_regions:
-	pci_release_regions(pdev);
-out_disable:
-	pci_disable_device(pdev);
-out_free:
-	kfree(bp);
-
-	return err;
 }
 
 static void
@@ -363,11 +346,6 @@ ptp_ocp_remove(struct pci_dev *pdev)
 	struct ptp_ocp *bp = pci_get_drvdata(pdev);
 
 	ptp_clock_unregister(bp->ptp);
-	pci_iounmap(pdev, bp->base);
-	pci_release_regions(pdev);
-	pci_disable_device(pdev);
-	pci_set_drvdata(pdev, NULL);
-	kfree(bp);
 }
 
 static struct pci_driver ptp_ocp_driver = {
-- 
2.30.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks
  2021-08-13 12:27 [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Andy Shevchenko
  2021-08-13 12:27 ` [PATCH v1 net-next 2/3] ptp_ocp: Convert to use managed functions pcim_* and devm_* Andy Shevchenko
@ 2021-08-13 12:27 ` Andy Shevchenko
  2021-08-13 17:35   ` kernel test robot
  2021-08-13 18:14 ` [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Jakub Kicinski
  2 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-13 12:27 UTC (permalink / raw)
  To: Jonathan Lemon, Andy Shevchenko, netdev, linux-kernel; +Cc: Richard Cochran

Currently we are using BIT(), but GENMASK(). Make use of the latter one
as well (far less error-prone, far more concise).

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/ptp/ptp_ocp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 5d0c0c0734f2..aad29b07c05b 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2020 Facebook */
 
+#include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -60,10 +61,10 @@ struct tod_reg {
 #define TOD_CTRL_DISABLE_FMT_A	BIT(17)
 #define TOD_CTRL_DISABLE_FMT_B	BIT(16)
 #define TOD_CTRL_ENABLE		BIT(0)
-#define TOD_CTRL_GNSS_MASK	((1U << 4) - 1)
+#define TOD_CTRL_GNSS_MASK	GENMASK(3, 0)
 #define TOD_CTRL_GNSS_SHIFT	24
 
-#define TOD_STATUS_UTC_MASK	0xff
+#define TOD_STATUS_UTC_MASK	GENMASK(7, 0)
 #define TOD_STATUS_UTC_VALID	BIT(8)
 #define TOD_STATUS_LEAP_VALID	BIT(16)
 
-- 
2.30.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks
  2021-08-13 12:27 ` [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks Andy Shevchenko
@ 2021-08-13 17:35   ` kernel test robot
  2021-08-13 17:48     ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: kernel test robot @ 2021-08-13 17:35 UTC (permalink / raw)
  To: Andy Shevchenko, Jonathan Lemon, netdev, linux-kernel
  Cc: kbuild-all, Richard Cochran

[-- Attachment #1: Type: text/plain, Size: 6657 bytes --]

Hi Andy,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.14-rc5]
[cannot apply to net-next/master next-20210813]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/ptp_ocp-Switch-to-use-module_pci_driver-macro/20210813-202935
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f8e6dfc64f6135d1b6c5215c14cd30b9b60a0008
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6366fc4d87438e21b0bdf4d4f680a5ec582740ad
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/ptp_ocp-Switch-to-use-module_pci_driver-macro/20210813-202935
        git checkout 6366fc4d87438e21b0bdf4d4f680a5ec582740ad
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/pci.h:37,
                    from drivers/ptp/ptp_ocp.c:8:
   drivers/ptp/ptp_ocp.c: In function 'ptp_ocp_tod_info':
>> drivers/ptp/ptp_ocp.c:276:34: warning: format '%d' expects argument of type 'int', but argument 3 has type 'long unsigned int' [-Wformat=]
     276 |         dev_info(&bp->pdev->dev, "utc_offset: %d  valid:%d  leap_valid:%d\n",
         |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
      19 | #define dev_fmt(fmt) fmt
         |                      ^~~
   drivers/ptp/ptp_ocp.c:276:9: note: in expansion of macro 'dev_info'
     276 |         dev_info(&bp->pdev->dev, "utc_offset: %d  valid:%d  leap_valid:%d\n",
         |         ^~~~~~~~
   drivers/ptp/ptp_ocp.c:276:48: note: format string is defined here
     276 |         dev_info(&bp->pdev->dev, "utc_offset: %d  valid:%d  leap_valid:%d\n",
         |                                               ~^
         |                                                |
         |                                                int
         |                                               %ld


vim +276 drivers/ptp/ptp_ocp.c

a7e1abad13f3f0 Jonathan Lemon 2020-12-03  234  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  235  static void
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  236  ptp_ocp_tod_info(struct ptp_ocp *bp)
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  237  {
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  238  	static const char * const proto_name[] = {
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  239  		"NMEA", "NMEA_ZDA", "NMEA_RMC", "NMEA_none",
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  240  		"UBX", "UBX_UTC", "UBX_LS", "UBX_none"
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  241  	};
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  242  	static const char * const gnss_name[] = {
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  243  		"ALL", "COMBINED", "GPS", "GLONASS", "GALILEO", "BEIDOU",
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  244  	};
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  245  	u32 version, ctrl, reg;
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  246  	int idx;
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  247  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  248  	version = ioread32(&bp->tod->version);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  249  	dev_info(&bp->pdev->dev, "TOD Version %d.%d.%d\n",
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  250  		 version >> 24, (version >> 16) & 0xff, version & 0xffff);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  251  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  252  	ctrl = ioread32(&bp->tod->ctrl);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  253  	ctrl |= TOD_CTRL_PROTOCOL | TOD_CTRL_ENABLE;
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  254  	ctrl &= ~(TOD_CTRL_DISABLE_FMT_A | TOD_CTRL_DISABLE_FMT_B);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  255  	iowrite32(ctrl, &bp->tod->ctrl);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  256  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  257  	ctrl = ioread32(&bp->tod->ctrl);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  258  	idx = ctrl & TOD_CTRL_PROTOCOL ? 4 : 0;
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  259  	idx += (ctrl >> 16) & 3;
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  260  	dev_info(&bp->pdev->dev, "control: %x\n", ctrl);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  261  	dev_info(&bp->pdev->dev, "TOD Protocol %s %s\n", proto_name[idx],
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  262  		 ctrl & TOD_CTRL_ENABLE ? "enabled" : "");
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  263  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  264  	idx = (ctrl >> TOD_CTRL_GNSS_SHIFT) & TOD_CTRL_GNSS_MASK;
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  265  	if (idx < ARRAY_SIZE(gnss_name))
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  266  		dev_info(&bp->pdev->dev, "GNSS %s\n", gnss_name[idx]);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  267  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  268  	reg = ioread32(&bp->tod->status);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  269  	dev_info(&bp->pdev->dev, "status: %x\n", reg);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  270  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  271  	reg = ioread32(&bp->tod->correction_sec);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  272  	dev_info(&bp->pdev->dev, "correction: %d\n", reg);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  273  
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  274  	reg = ioread32(&bp->tod->utc_status);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  275  	dev_info(&bp->pdev->dev, "utc_status: %x\n", reg);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03 @276  	dev_info(&bp->pdev->dev, "utc_offset: %d  valid:%d  leap_valid:%d\n",
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  277  		 reg & TOD_STATUS_UTC_MASK, reg & TOD_STATUS_UTC_VALID ? 1 : 0,
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  278  		 reg & TOD_STATUS_LEAP_VALID ? 1 : 0);
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  279  }
a7e1abad13f3f0 Jonathan Lemon 2020-12-03  280  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 68432 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks
  2021-08-13 17:35   ` kernel test robot
@ 2021-08-13 17:48     ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-13 17:48 UTC (permalink / raw)
  To: kernel test robot
  Cc: Jonathan Lemon, netdev, linux-kernel, kbuild-all, Richard Cochran

On Sat, Aug 14, 2021 at 01:35:13AM +0800, kernel test robot wrote:
> Hi Andy,
> 
> I love your patch! Perhaps something to improve:

Not in my patches.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro
  2021-08-13 12:27 [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Andy Shevchenko
  2021-08-13 12:27 ` [PATCH v1 net-next 2/3] ptp_ocp: Convert to use managed functions pcim_* and devm_* Andy Shevchenko
  2021-08-13 12:27 ` [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks Andy Shevchenko
@ 2021-08-13 18:14 ` Jakub Kicinski
  2021-08-13 19:30   ` Andy Shevchenko
  2 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2021-08-13 18:14 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: Jonathan Lemon, netdev, linux-kernel, Richard Cochran

On Fri, 13 Aug 2021 15:27:35 +0300 Andy Shevchenko wrote:
> Eliminate some boilerplate code by using module_pci_driver() instead of
> init/exit, and, if needed, moving the salient bits from init into probe.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Jonathan has a series in flight which is fixing some of the same issues:
https://patchwork.kernel.org/project/netdevbpf/list/?series=530079&state=*

Please hold off for a day or two so it can get merged, and if you don't
mind double check at that point which of your patches are still needed.

According to patchwork your series does not apply to net-next as of
last night so it'll need a respin anyway.

Thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro
  2021-08-13 18:14 ` [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Jakub Kicinski
@ 2021-08-13 19:30   ` Andy Shevchenko
  2021-08-16 21:01     ` Jonathan Lemon
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-13 19:30 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Andy Shevchenko, Jonathan Lemon, netdev,
	Linux Kernel Mailing List, Richard Cochran

On Fri, Aug 13, 2021 at 9:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 13 Aug 2021 15:27:35 +0300 Andy Shevchenko wrote:
> > Eliminate some boilerplate code by using module_pci_driver() instead of
> > init/exit, and, if needed, moving the salient bits from init into probe.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Jonathan has a series in flight which is fixing some of the same issues:
> https://patchwork.kernel.org/project/netdevbpf/list/?series=530079&state=*
>
> Please hold off for a day or two so it can get merged, and if you don't
> mind double check at that point which of your patches are still needed.

Actually it may be the other way around. Since patch 2 in his series
is definitely an unneeded churn here, because my devm conversion will
have to effectively revert it.


> According to patchwork your series does not apply to net-next as of
> last night so it'll need a respin anyway.

I hope he will chime in and see what we can do the best.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro
  2021-08-13 19:30   ` Andy Shevchenko
@ 2021-08-16 21:01     ` Jonathan Lemon
  2021-08-17  9:48       ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Lemon @ 2021-08-16 21:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jakub Kicinski, Andy Shevchenko, netdev,
	Linux Kernel Mailing List, Richard Cochran

On Fri, Aug 13, 2021 at 10:30:51PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 13, 2021 at 9:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Fri, 13 Aug 2021 15:27:35 +0300 Andy Shevchenko wrote:
> > > Eliminate some boilerplate code by using module_pci_driver() instead of
> > > init/exit, and, if needed, moving the salient bits from init into probe.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Jonathan has a series in flight which is fixing some of the same issues:
> > https://patchwork.kernel.org/project/netdevbpf/list/?series=530079&state=*
> >
> > Please hold off for a day or two so it can get merged, and if you don't
> > mind double check at that point which of your patches are still needed.
> 
> Actually it may be the other way around. Since patch 2 in his series
> is definitely an unneeded churn here, because my devm conversion will
> have to effectively revert it.
> 
> 
> > According to patchwork your series does not apply to net-next as of
> > last night so it'll need a respin anyway.
> 
> I hope he will chime in and see what we can do the best.

I'm going to submit a respin of the last patch, I screwed something
up from all the various trees I'm using.

Please update to net-next first - the firat patch in your series
doesn't make any longer, given the current status.
-- 
Jonathan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro
  2021-08-16 21:01     ` Jonathan Lemon
@ 2021-08-17  9:48       ` Andy Shevchenko
  2021-08-17 12:43         ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-17  9:48 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: Jakub Kicinski, netdev, Linux Kernel Mailing List, Richard Cochran

On Mon, Aug 16, 2021 at 02:01:01PM -0700, Jonathan Lemon wrote:
> On Fri, Aug 13, 2021 at 10:30:51PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 13, 2021 at 9:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Fri, 13 Aug 2021 15:27:35 +0300 Andy Shevchenko wrote:
> > > > Eliminate some boilerplate code by using module_pci_driver() instead of
> > > > init/exit, and, if needed, moving the salient bits from init into probe.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > Jonathan has a series in flight which is fixing some of the same issues:
> > > https://patchwork.kernel.org/project/netdevbpf/list/?series=530079&state=*
> > >
> > > Please hold off for a day or two so it can get merged, and if you don't
> > > mind double check at that point which of your patches are still needed.
> > 
> > Actually it may be the other way around. Since patch 2 in his series
> > is definitely an unneeded churn here, because my devm conversion will
> > have to effectively revert it.
> > 
> > 
> > > According to patchwork your series does not apply to net-next as of
> > > last night so it'll need a respin anyway.
> > 
> > I hope he will chime in and see what we can do the best.
> 
> I'm going to submit a respin of the last patch, I screwed something
> up from all the various trees I'm using.
> 
> Please update to net-next first - the firat patch in your series
> doesn't make any longer, given the current status.

I'll rebase my stuff on top of net-next and resubmit.

Thanks!


-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro
  2021-08-17  9:48       ` Andy Shevchenko
@ 2021-08-17 12:43         ` Andy Shevchenko
  0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2021-08-17 12:43 UTC (permalink / raw)
  To: Jonathan Lemon
  Cc: Jakub Kicinski, netdev, Linux Kernel Mailing List, Richard Cochran

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

On Tue, Aug 17, 2021 at 12:48:22PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 16, 2021 at 02:01:01PM -0700, Jonathan Lemon wrote:
> > On Fri, Aug 13, 2021 at 10:30:51PM +0300, Andy Shevchenko wrote:
> > > On Fri, Aug 13, 2021 at 9:15 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > On Fri, 13 Aug 2021 15:27:35 +0300 Andy Shevchenko wrote:
> > > > > Eliminate some boilerplate code by using module_pci_driver() instead of
> > > > > init/exit, and, if needed, moving the salient bits from init into probe.
> > > > >
> > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > >
> > > > Jonathan has a series in flight which is fixing some of the same issues:
> > > > https://patchwork.kernel.org/project/netdevbpf/list/?series=530079&state=*
> > > >
> > > > Please hold off for a day or two so it can get merged, and if you don't
> > > > mind double check at that point which of your patches are still needed.
> > > 
> > > Actually it may be the other way around. Since patch 2 in his series
> > > is definitely an unneeded churn here, because my devm conversion will
> > > have to effectively revert it.
> > > 
> > > 
> > > > According to patchwork your series does not apply to net-next as of
> > > > last night so it'll need a respin anyway.
> > > 
> > > I hope he will chime in and see what we can do the best.
> > 
> > I'm going to submit a respin of the last patch, I screwed something
> > up from all the various trees I'm using.
> > 
> > Please update to net-next first - the firat patch in your series
> > doesn't make any longer, given the current status.
> 
> I'll rebase my stuff on top of net-next and resubmit.
> 
> Thanks!

It seems the driver disrupted so much that it requires much more work
to make it neat. New code looks like a custom MFD approach (WRT resource
management).

I have sent only patch 3 out of this series and have attached here the
problematic places in my opinion. Feel free to convert them to patches
with Suggested-by tag. But converting to MFD will make this driver much
much better to read, understand and maintain.


-- 
With Best Regards,
Andy Shevchenko



[-- Attachment #2: 0001-TODO-ptp_ocp-Convert-to-use-managed-functions-pcim_-.patch --]
[-- Type: text/x-diff, Size: 6264 bytes --]

From b8b54ce18d724fd2c730b553a67c77f2c4a0fcf2 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Tue, 17 Aug 2021 15:25:38 +0300
Subject: [PATCH 1/1] TODO: ptp_ocp: Convert to use managed functions pcim_*
 and devm_*

This makes the error handling much more simpler than open-coding everything
and in addition makes the probe function smaller an tidier.

TODO: split out unrelated changes to their own patches.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/ptp/ptp_ocp.c | 56 ++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 922f92637db8..d118da95a46c 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2020 Facebook */
 
+#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
+
 #include <linux/bits.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
@@ -303,7 +305,7 @@ static struct ocp_resource ocp_fb_resource[] = {
 
 static const struct pci_device_id ptp_ocp_pcidev_id[] = {
 	{ PCI_DEVICE_DATA(FACEBOOK, TIMECARD, &ocp_fb_resource) },
-	{ 0 }
+	{ }
 };
 MODULE_DEVICE_TABLE(pci, ptp_ocp_pcidev_id);
 
@@ -344,6 +346,7 @@ ptp_ocp_clock_val_from_name(const char *name)
 
 	for (i = 0; i < ARRAY_SIZE(ptp_ocp_clock); i++) {
 		clk = ptp_ocp_clock[i].name;
+		/* FIXME: What's the point of 'n' + strlen()? */
 		if (!strncasecmp(name, clk, strlen(clk)))
 			return ptp_ocp_clock[i].value;
 	}
@@ -363,6 +366,7 @@ __ptp_ocp_gettime_locked(struct ptp_ocp *bp, struct timespec64 *ts,
 	ptp_read_system_prets(sts);
 	iowrite32(ctrl, &bp->reg->ctrl);
 
+	/* FIXME: iopoll.h + respective macro */
 	for (i = 0; i < 100; i++) {
 		ctrl = ioread32(&bp->reg->ctrl);
 		if (ctrl & OCP_CTRL_READ_TIME_DONE)
@@ -686,6 +690,9 @@ ptp_ocp_read_i2c(struct i2c_adapter *adap, u8 addr, u8 reg, u8 sz, u8 *data)
 		reg += len;
 		sz -= len;
 	}
+
+	/* FIXME: shouldn't be using word transfers then? */
+
 	return 0;
 }
 
@@ -870,21 +877,13 @@ static const struct devlink_ops ptp_ocp_devlink_ops = {
 	.info_get = ptp_ocp_devlink_info_get,
 };
 
-static void __iomem *
-__ptp_ocp_get_mem(struct ptp_ocp *bp, unsigned long start, int size)
-{
-	struct resource res = DEFINE_RES_MEM_NAMED(start, size, "ptp_ocp");
-
-	return devm_ioremap_resource(&bp->pdev->dev, &res);
-}
-
 static void __iomem *
 ptp_ocp_get_mem(struct ptp_ocp *bp, struct ocp_resource *r)
 {
-	unsigned long start;
+	resource_size_t start = pci_resource_start(bp->pdev, 0) + r->offset;
+	struct resource res = DEFINE_RES_MEM_NAMED(start, r->size, "ptp_ocp");
 
-	start = pci_resource_start(bp->pdev, 0) + r->offset;
-	return __ptp_ocp_get_mem(bp, start, r->size);
+	return devm_ioremap_resource(&bp->pdev->dev, &res);
 }
 
 static void
@@ -908,7 +907,7 @@ ptp_ocp_register_spi(struct ptp_ocp *bp, struct ocp_resource *r)
 	struct pci_dev *pdev = bp->pdev;
 	struct platform_device *p;
 	struct resource res[2];
-	unsigned long start;
+	resource_size_t start;
 	int id;
 
 	/* XXX hack to work around old FPGA */
@@ -932,7 +931,7 @@ ptp_ocp_register_spi(struct ptp_ocp *bp, struct ocp_resource *r)
 	id += info->pci_offset;
 
 	p = platform_device_register_resndata(&pdev->dev, info->name, id,
-					      res, 2, info->data,
+					      res, ARRAY_SIZE(res), info->data,
 					      info->data_size);
 	if (IS_ERR(p))
 		return PTR_ERR(p);
@@ -1036,7 +1035,6 @@ ptp_ocp_unregister_ext(struct ptp_ocp_ext_src *ext)
 {
 	ext->info->enable(ext, false);
 	pci_free_irq(ext->bp->pdev, ext->irq_vec, ext);
-	kfree(ext);
 }
 
 static int
@@ -1046,14 +1044,13 @@ ptp_ocp_register_ext(struct ptp_ocp *bp, struct ocp_resource *r)
 	struct ptp_ocp_ext_src *ext;
 	int err;
 
-	ext = kzalloc(sizeof(*ext), GFP_KERNEL);
+	ext = devm_kzalloc(&pdev->dev, sizeof(*ext), GFP_KERNEL);
 	if (!ext)
 		return -ENOMEM;
 
-	err = -EINVAL;
 	ext->mem = ptp_ocp_get_mem(bp, r);
 	if (!ext->mem)
-		goto out;
+		return -EINVAL;
 
 	ext->bp = bp;
 	ext->info = r->extra;
@@ -1063,16 +1060,12 @@ ptp_ocp_register_ext(struct ptp_ocp *bp, struct ocp_resource *r)
 			      ext, "ocp%d.%s", bp->id, ext->info->name);
 	if (err) {
 		dev_err(&pdev->dev, "Could not get irq %d\n", r->irq_vec);
-		goto out;
+		return err;
 	}
 
 	bp_assign_entry(bp, r, ext);
 
 	return 0;
-
-out:
-	kfree(ext);
-	return err;
 }
 
 static int
@@ -1240,7 +1233,7 @@ static struct attribute *timecard_attrs[] = {
 	&dev_attr_gnss_sync.attr,
 	&dev_attr_clock_source.attr,
 	&dev_attr_available_clock_sources.attr,
-	NULL,
+	NULL
 };
 ATTRIBUTE_GROUPS(timecard);
 
@@ -1430,7 +1423,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (err)
 		goto out_free;
 
-	err = pci_enable_device(pdev);
+	err = pcim_enable_device(pdev);
 	if (err) {
 		dev_err(&pdev->dev, "pci_enable_device\n");
 		goto out_unregister;
@@ -1439,7 +1432,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	bp = devlink_priv(devlink);
 	err = ptp_ocp_device_init(bp, pdev);
 	if (err)
-		goto out_disable;
+		goto out_unregister;
 
 	/* compat mode.
 	 * Older FPGA firmware only returns 2 irq's.
@@ -1456,7 +1449,7 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	err = ptp_ocp_register_resources(bp, id->driver_data);
 	if (err)
-		goto out;
+		goto out_free_irq;
 
 	bp->ptp = ptp_clock_register(&bp->ptp_info, &pdev->dev);
 	if (IS_ERR(bp->ptp)) {
@@ -1477,9 +1470,8 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 out:
 	ptp_ocp_detach(bp);
-	pci_set_drvdata(pdev, NULL);
-out_disable:
-	pci_disable_device(pdev);
+out_free_irq:
+	pci_free_irq_vectors(pdev);
 out_unregister:
 	devlink_unregister(devlink);
 out_free:
@@ -1495,8 +1487,6 @@ ptp_ocp_remove(struct pci_dev *pdev)
 	struct devlink *devlink = priv_to_devlink(bp);
 
 	ptp_ocp_detach(bp);
-	pci_set_drvdata(pdev, NULL);
-	pci_disable_device(pdev);
 
 	devlink_unregister(devlink);
 	devlink_free(devlink);
@@ -1577,7 +1567,7 @@ ptp_ocp_init(void)
 out_notifier:
 	class_unregister(&timecard_class);
 out:
-	pr_err(KBUILD_MODNAME ": failed to register %s: %d\n", what, err);
+	pr_err("failed to register %s: %d\n", what, err);
 	return err;
 }
 
-- 
2.32.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-17 12:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 12:27 [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Andy Shevchenko
2021-08-13 12:27 ` [PATCH v1 net-next 2/3] ptp_ocp: Convert to use managed functions pcim_* and devm_* Andy Shevchenko
2021-08-13 12:27 ` [PATCH v1 net-next 3/3] ptp_ocp: use bits.h macros for all masks Andy Shevchenko
2021-08-13 17:35   ` kernel test robot
2021-08-13 17:48     ` Andy Shevchenko
2021-08-13 18:14 ` [PATCH v1 net-next 1/3] ptp_ocp: Switch to use module_pci_driver() macro Jakub Kicinski
2021-08-13 19:30   ` Andy Shevchenko
2021-08-16 21:01     ` Jonathan Lemon
2021-08-17  9:48       ` Andy Shevchenko
2021-08-17 12:43         ` Andy Shevchenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).