[PATCH 0/5] staging: kpc2000: assorted fixes

[PATCH 0/5] staging: kpc2000: assorted fixes

[Jeremy Sowden]

The first patch contains some white-space and formatting fixes that I
made while I was looking at the following two TODO items:

  * the loop in kp2000_probe_cores() that uses probe_core_uio() also
    probably needs error handling.
  * probe_core_uio() probably needs error handling

The second and third patches contain fixes for some sparse warnings.

The last two patches contain the actual error-handling fixes.

Jeremy Sowden (5):
  staging: kpc2000: inverted conditional in order to reduce indentation.
  staging: kpc2000: declare two functions as static.
  staging: kpc2000: added designated initializers to two structs.
  staging: kpc2000: added missing clean-up to probe_core_uio.
  staging: kpc2000: clean up after probe failure.

 drivers/staging/kpc2000/TODO                 |  2 -
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 63 ++++++++++++--------
 2 files changed, 39 insertions(+), 26 deletions(-)

-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/5] staging: kpc2000: inverted conditional in order to reduce indentation.

[Jeremy Sowden]

Changed:

  for (...) {
    ...
    if (expr) {
      ...
    }
  }

into:

  for (...) {
    ...
    if (!expr)
      continue;
    ...
  }

in order to reduce indentation of conditional block.  Fixed indentation
of cases blocks at the same time.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 43 +++++++++++---------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index e0dba91e7fa8..fc7038152bb4 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -414,26 +414,31 @@ int  kp2000_probe_cores(struct kp2000_device *pcard)
             read_val = readq(pcard->sysinfo_regs_base + ((pcard->core_table_offset + i) * 8));
             parse_core_table_entry(&cte, read_val, pcard->core_table_rev);
             
-            if (cte.type == current_type_id){
-                switch (cte.type){
-                    case KP_CORE_ID_I2C:
-                        err = probe_core_basic(core_num, pcard, KP_DRIVER_NAME_I2C, cte);
-                        break;
-                    
-                    case KP_CORE_ID_SPI:
-                        err = probe_core_basic(core_num, pcard, KP_DRIVER_NAME_SPI, cte);
-                        break;
-                    
-                    default:
-                        err = probe_core_uio(core_num, pcard, "kpc_uio", cte);
-                        break;
-                }
-                if (err){
-                    dev_err(&pcard->pdev->dev, "kp2000_probe_cores: failed to add core %d: %d\n", i, err);
-                    return err;
-                }
-                core_num++;
+            if (cte.type != current_type_id)
+                continue;
+
+            switch (cte.type) {
+            case KP_CORE_ID_I2C:
+                err = probe_core_basic(core_num, pcard,
+                                       KP_DRIVER_NAME_I2C, cte);
+                break;
+
+            case KP_CORE_ID_SPI:
+                err = probe_core_basic(core_num, pcard,
+                                       KP_DRIVER_NAME_SPI, cte);
+                break;
+
+            default:
+                err = probe_core_uio(core_num, pcard, "kpc_uio", cte);
+                break;
             }
+            if (err) {
+                dev_err(&pcard->pdev->dev,
+                        "kp2000_probe_cores: failed to add core %d: %d\n",
+                        i, err);
+                return err;
+            }
+            core_num++;
         }
     }
     
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/5] staging: kpc2000: declare two functions as static.

[Jeremy Sowden]

Two functions were not used outside the translation-unit in which they
were defined.  Declared them static.

Fixes two sparse warnings:

  drivers/staging/kpc2000/kpc2000/cell_probe.c:98:5: warning: symbol 'probe_core_basic' was not declared. Should it be static?
  drivers/staging/kpc2000/kpc2000/cell_probe.c:288:5: warning: symbol 'probe_core_uio' was not declared. Should it be static?

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index fc7038152bb4..30e6f176ddfa 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -91,7 +91,8 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
 }
 
 
-int  probe_core_basic(unsigned int core_num, struct kp2000_device *pcard, char *name, const struct core_table_entry cte)
+static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
+			    char *name, const struct core_table_entry cte)
 {
     struct mfd_cell  cell = {0};
     struct resource  resources[2];
@@ -257,7 +258,8 @@ int kuio_irqcontrol(struct uio_info *uioinfo, s32 irq_on)
     return 0;
 }
 
-int  probe_core_uio(unsigned int core_num, struct kp2000_device *pcard, char *name, const struct core_table_entry cte)
+static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
+			  char *name, const struct core_table_entry cte)
 {
     struct kpc_uio_device  *kudev;
     int rv;
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 3/5] staging: kpc2000: added designated initializers to two structs.

[Jeremy Sowden]

Fixed the following two sparse warnings by using designated
initializers:

  drivers/staging/kpc2000/kpc2000/cell_probe.c:101:34: warning: Using plain integer as NULL pointer
  drivers/staging/kpc2000/kpc2000/cell_probe.c:364:34: warning: Using plain integer as NULL pointer

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index 30e6f176ddfa..9cb745f4323a 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
 static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
 			    char *name, const struct core_table_entry cte)
 {
-    struct mfd_cell  cell = {0};
+    struct mfd_cell  cell = { .id = core_num, .name = name };
     struct resource  resources[2];
 
     struct kpc_core_device_platdata  core_pdata = {
@@ -315,7 +315,7 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
 
 static int  create_dma_engine_core(struct kp2000_device *pcard, size_t engine_regs_offset, int engine_num, int irq_num)
 {
-    struct mfd_cell  cell = {0};
+    struct mfd_cell  cell = { .id = engine_num };
     struct resource  resources[2];
 
     dev_dbg(&pcard->pdev->dev, "create_dma_core(pcard = [%p], engine_regs_offset = %zx, engine_num = %d)\n", pcard, engine_regs_offset, engine_num);
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 4/5] staging: kpc2000: added missing clean-up to probe_core_uio.

[Jeremy Sowden]

On error, probe_core_uio just returned an error without freeing
resources which had previously been allocated.  Added the missing
clean-up code.

Updated TODO.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 drivers/staging/kpc2000/TODO                 | 1 -
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/kpc2000/TODO b/drivers/staging/kpc2000/TODO
index 8c7af29fefae..ed951acc829a 100644
--- a/drivers/staging/kpc2000/TODO
+++ b/drivers/staging/kpc2000/TODO
@@ -1,7 +1,6 @@
 - the kpc_spi driver doesn't seem to let multiple transactions (to different instances of the core) happen in parallel...
 - The kpc_i2c driver is a hot mess, it should probably be cleaned up a ton.  It functions against current hardware though.
 - pcard->card_num in kp2000_pcie_probe() is a global variable and needs atomic / locking / something better.
-- probe_core_uio() probably needs error handling
 - the loop in kp2000_probe_cores() that uses probe_core_uio() also probably needs error handling
 - would be nice if the AIO fileops in kpc_dma could be made to work
     - probably want to add a CONFIG_ option to control compilation of the AIO functions
diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index 9cb745f4323a..211fe2da6daf 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -297,6 +297,7 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
     kudev->dev = device_create(kpc_uio_class, &pcard->pdev->dev, MKDEV(0,0), kudev, "%s.%d.%d.%d", kudev->uioinfo.name, pcard->card_num, cte.type, kudev->core_num);
     if (IS_ERR(kudev->dev)) {
         dev_err(&pcard->pdev->dev, "probe_core_uio device_create failed!\n");
+        kfree(kudev);
         return -ENODEV;
     }
     dev_set_drvdata(kudev->dev, kudev);
@@ -304,6 +305,8 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
     rv = uio_register_device(kudev->dev, &kudev->uioinfo);
     if (rv){
         dev_err(&pcard->pdev->dev, "probe_core_uio failed uio_register_device: %d\n", rv);
+        put_device(kudev->dev);
+        kfree(kudev);
         return rv;
     }
     
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 5/5] staging: kpc2000: clean up after probe failure.

[Jeremy Sowden]

On error, kp2000_probe_cores just returned an error without freeing
resources which had previously been allocated.  Added the missing
clean-up code.

Updated TODO.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 drivers/staging/kpc2000/TODO                 | 1 -
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 9 +++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/kpc2000/TODO b/drivers/staging/kpc2000/TODO
index ed951acc829a..669fe5bf9637 100644
--- a/drivers/staging/kpc2000/TODO
+++ b/drivers/staging/kpc2000/TODO
@@ -1,7 +1,6 @@
 - the kpc_spi driver doesn't seem to let multiple transactions (to different instances of the core) happen in parallel...
 - The kpc_i2c driver is a hot mess, it should probably be cleaned up a ton.  It functions against current hardware though.
 - pcard->card_num in kp2000_pcie_probe() is a global variable and needs atomic / locking / something better.
-- the loop in kp2000_probe_cores() that uses probe_core_uio() also probably needs error handling
 - would be nice if the AIO fileops in kpc_dma could be made to work
     - probably want to add a CONFIG_ option to control compilation of the AIO functions
 - if the AIO fileops in kpc_dma start working, next would be making iov_count > 1 work too
diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index 211fe2da6daf..6f88c78334bc 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -441,7 +441,7 @@ int  kp2000_probe_cores(struct kp2000_device *pcard)
                 dev_err(&pcard->pdev->dev,
                         "kp2000_probe_cores: failed to add core %d: %d\n",
                         i, err);
-                return err;
+                goto error;
             }
             core_num++;
         }
@@ -460,10 +460,15 @@ int  kp2000_probe_cores(struct kp2000_device *pcard)
     err = probe_core_uio(0, pcard, "kpc_uio", cte);
     if (err){
         dev_err(&pcard->pdev->dev, "kp2000_probe_cores: failed to add board_info core: %d\n", err);
-        return err;
+        goto error;
     }
     
     return 0;
+
+error:
+    kp2000_remove_cores(pcard);
+    mfd_remove_devices(PCARD_TO_DEV(pcard));
+    return err;
 }
 
 void  kp2000_remove_cores(struct kp2000_device *pcard)
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: kpc2000: added designated initializers to two structs.

[Dan Carpenter]

On Wed, May 15, 2019 at 11:34:52AM +0100, Jeremy Sowden wrote:
> Fixed the following two sparse warnings by using designated
> initializers:
> 
>   drivers/staging/kpc2000/kpc2000/cell_probe.c:101:34: warning: Using plain integer as NULL pointer
>   drivers/staging/kpc2000/kpc2000/cell_probe.c:364:34: warning: Using plain integer as NULL pointer
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  drivers/staging/kpc2000/kpc2000/cell_probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> index 30e6f176ddfa..9cb745f4323a 100644
> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> @@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
>  static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
>  			    char *name, const struct core_table_entry cte)
>  {
> -    struct mfd_cell  cell = {0};
> +    struct mfd_cell  cell = { .id = core_num, .name = name };
>      struct resource  resources[2];
>  
>      struct kpc_core_device_platdata  core_pdata = {
> @@ -315,7 +315,7 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
>  
>  static int  create_dma_engine_core(struct kp2000_device *pcard, size_t engine_regs_offset, int engine_num, int irq_num)
>  {
> -    struct mfd_cell  cell = {0};
> +    struct mfd_cell  cell = { .id = engine_num };
>      struct resource  resources[2];
>  

These changes make no sense because we just write over it later.

Maybe you're going to fix it up later in the patch series, perhaps but
that's not how it's done.  Each patch should do "one thing", not "half
and thing and then half a thing later in the series possibly (I am
reviewing the patches in order so I don't know)".

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: kpc2000: added designated initializers to two structs.

[Dan Carpenter]

On Wed, May 15, 2019 at 02:09:49PM +0300, Dan Carpenter wrote:
> On Wed, May 15, 2019 at 11:34:52AM +0100, Jeremy Sowden wrote:
> > Fixed the following two sparse warnings by using designated
> > initializers:
> > 
> >   drivers/staging/kpc2000/kpc2000/cell_probe.c:101:34: warning: Using plain integer as NULL pointer
> >   drivers/staging/kpc2000/kpc2000/cell_probe.c:364:34: warning: Using plain integer as NULL pointer
> > 
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  drivers/staging/kpc2000/kpc2000/cell_probe.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > index 30e6f176ddfa..9cb745f4323a 100644
> > --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > @@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
> >  static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
> >  			    char *name, const struct core_table_entry cte)
> >  {
> > -    struct mfd_cell  cell = {0};
> > +    struct mfd_cell  cell = { .id = core_num, .name = name };
> >      struct resource  resources[2];
> >  
> >      struct kpc_core_device_platdata  core_pdata = {
> > @@ -315,7 +315,7 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
> >  
> >  static int  create_dma_engine_core(struct kp2000_device *pcard, size_t engine_regs_offset, int engine_num, int irq_num)
> >  {
> > -    struct mfd_cell  cell = {0};
> > +    struct mfd_cell  cell = { .id = engine_num };
> >      struct resource  resources[2];
> >  
> 
> These changes make no sense because we just write over it later.
> 
> Maybe you're going to fix it up later in the patch series, perhaps but
> that's not how it's done.  Each patch should do "one thing", not "half
> and thing and then half a thing later in the series possibly (I am
> reviewing the patches in order so I don't know)".
> 

I've finished reviewing the series and we never complete the other half
of this patch.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 0/5] staging: kpc2000: assorted fixes

[Jeremy Sowden]

The first patch contains some white-space and formatting fixes that I
made while I was looking at the following two TODO items:

  * the loop in kp2000_probe_cores() that uses probe_core_uio() also
    probably needs error handling.
  * probe_core_uio() probably needs error handling

The second and third patches contain fixes for some sparse warnings.

The last two patches contain the actual error-handling fixes.

Jeremy Sowden (5):
  staging: kpc2000: inverted conditional in order to reduce indentation.
  staging: kpc2000: declare two functions as static.
  staging: kpc2000: added designated initializers to two structs.
  staging: kpc2000: added missing clean-up to probe_core_uio.
  staging: kpc2000: clean up after probe failure.

 drivers/staging/kpc2000/TODO                 |  2 -
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 66 ++++++++++++--------
 2 files changed, 39 insertions(+), 29 deletions(-)

Since v1:

  * in patch 3 removed the assignment of the struct-members initialized
    by the designated initializers.

-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/5] staging: kpc2000: inverted conditional in order to reduce indentation.

[Jeremy Sowden]

Changed:

  for (...) {
    ...
    if (expr) {
      ...
    }
  }

into:

  for (...) {
    ...
    if (!expr)
      continue;
    ...
  }

in order to reduce indentation of conditional block.  Fixed indentation
of cases blocks at the same time.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 43 +++++++++++---------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index e0dba91e7fa8..fc7038152bb4 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -414,26 +414,31 @@ int  kp2000_probe_cores(struct kp2000_device *pcard)
             read_val = readq(pcard->sysinfo_regs_base + ((pcard->core_table_offset + i) * 8));
             parse_core_table_entry(&cte, read_val, pcard->core_table_rev);
             
-            if (cte.type == current_type_id){
-                switch (cte.type){
-                    case KP_CORE_ID_I2C:
-                        err = probe_core_basic(core_num, pcard, KP_DRIVER_NAME_I2C, cte);
-                        break;
-                    
-                    case KP_CORE_ID_SPI:
-                        err = probe_core_basic(core_num, pcard, KP_DRIVER_NAME_SPI, cte);
-                        break;
-                    
-                    default:
-                        err = probe_core_uio(core_num, pcard, "kpc_uio", cte);
-                        break;
-                }
-                if (err){
-                    dev_err(&pcard->pdev->dev, "kp2000_probe_cores: failed to add core %d: %d\n", i, err);
-                    return err;
-                }
-                core_num++;
+            if (cte.type != current_type_id)
+                continue;
+
+            switch (cte.type) {
+            case KP_CORE_ID_I2C:
+                err = probe_core_basic(core_num, pcard,
+                                       KP_DRIVER_NAME_I2C, cte);
+                break;
+
+            case KP_CORE_ID_SPI:
+                err = probe_core_basic(core_num, pcard,
+                                       KP_DRIVER_NAME_SPI, cte);
+                break;
+
+            default:
+                err = probe_core_uio(core_num, pcard, "kpc_uio", cte);
+                break;
             }
+            if (err) {
+                dev_err(&pcard->pdev->dev,
+                        "kp2000_probe_cores: failed to add core %d: %d\n",
+                        i, err);
+                return err;
+            }
+            core_num++;
         }
     }
     
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/5] staging: kpc2000: declare two functions as static.

[Jeremy Sowden]

Two functions were not used outside the translation-unit in which they
were defined.  Declared them static.

Fixes two sparse warnings:

  drivers/staging/kpc2000/kpc2000/cell_probe.c:98:5: warning: symbol 'probe_core_basic' was not declared. Should it be static?
  drivers/staging/kpc2000/kpc2000/cell_probe.c:288:5: warning: symbol 'probe_core_uio' was not declared. Should it be static?

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index fc7038152bb4..30e6f176ddfa 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -91,7 +91,8 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
 }
 
 
-int  probe_core_basic(unsigned int core_num, struct kp2000_device *pcard, char *name, const struct core_table_entry cte)
+static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
+			    char *name, const struct core_table_entry cte)
 {
     struct mfd_cell  cell = {0};
     struct resource  resources[2];
@@ -257,7 +258,8 @@ int kuio_irqcontrol(struct uio_info *uioinfo, s32 irq_on)
     return 0;
 }
 
-int  probe_core_uio(unsigned int core_num, struct kp2000_device *pcard, char *name, const struct core_table_entry cte)
+static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
+			  char *name, const struct core_table_entry cte)
 {
     struct kpc_uio_device  *kudev;
     int rv;
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/5] staging: kpc2000: added designated initializers to two structs.

[Jeremy Sowden]

Fixed the following two sparse warnings by using designated
initializers:

  drivers/staging/kpc2000/kpc2000/cell_probe.c:101:34: warning: Using plain integer as NULL pointer
  drivers/staging/kpc2000/kpc2000/cell_probe.c:364:34: warning: Using plain integer as NULL pointer

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index 30e6f176ddfa..b98d53c8637f 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
 static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
 			    char *name, const struct core_table_entry cte)
 {
-    struct mfd_cell  cell = {0};
+    struct mfd_cell  cell = { .id = core_num, .name = name };
     struct resource  resources[2];
 
     struct kpc_core_device_platdata  core_pdata = {
@@ -110,8 +110,6 @@ static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
     
     cell.platform_data = &core_pdata;
     cell.pdata_size = sizeof(struct kpc_core_device_platdata);
-    cell.name = name;
-    cell.id = core_num;
     cell.num_resources = 2;
     
     memset(&resources, 0, sizeof(resources));
@@ -315,14 +313,13 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
 
 static int  create_dma_engine_core(struct kp2000_device *pcard, size_t engine_regs_offset, int engine_num, int irq_num)
 {
-    struct mfd_cell  cell = {0};
+    struct mfd_cell  cell = { .id = engine_num };
     struct resource  resources[2];
 
     dev_dbg(&pcard->pdev->dev, "create_dma_core(pcard = [%p], engine_regs_offset = %zx, engine_num = %d)\n", pcard, engine_regs_offset, engine_num);
     
     cell.platform_data = NULL;
     cell.pdata_size = 0;
-    cell.id = engine_num;
     cell.name = KP_DRIVER_NAME_DMA_CONTROLLER;
     cell.num_resources = 2;
     
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 4/5] staging: kpc2000: added missing clean-up to probe_core_uio.

[Jeremy Sowden]

On error, probe_core_uio just returned an error without freeing
resources which had previously been allocated.  Added the missing
clean-up code.

Updated TODO.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/kpc2000/TODO                 | 1 -
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 3 +++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/kpc2000/TODO b/drivers/staging/kpc2000/TODO
index 8c7af29fefae..ed951acc829a 100644
--- a/drivers/staging/kpc2000/TODO
+++ b/drivers/staging/kpc2000/TODO
@@ -1,7 +1,6 @@
 - the kpc_spi driver doesn't seem to let multiple transactions (to different instances of the core) happen in parallel...
 - The kpc_i2c driver is a hot mess, it should probably be cleaned up a ton.  It functions against current hardware though.
 - pcard->card_num in kp2000_pcie_probe() is a global variable and needs atomic / locking / something better.
-- probe_core_uio() probably needs error handling
 - the loop in kp2000_probe_cores() that uses probe_core_uio() also probably needs error handling
 - would be nice if the AIO fileops in kpc_dma could be made to work
     - probably want to add a CONFIG_ option to control compilation of the AIO functions
diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index b98d53c8637f..e4c21291fe16 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -295,6 +295,7 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
     kudev->dev = device_create(kpc_uio_class, &pcard->pdev->dev, MKDEV(0,0), kudev, "%s.%d.%d.%d", kudev->uioinfo.name, pcard->card_num, cte.type, kudev->core_num);
     if (IS_ERR(kudev->dev)) {
         dev_err(&pcard->pdev->dev, "probe_core_uio device_create failed!\n");
+        kfree(kudev);
         return -ENODEV;
     }
     dev_set_drvdata(kudev->dev, kudev);
@@ -302,6 +303,8 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
     rv = uio_register_device(kudev->dev, &kudev->uioinfo);
     if (rv){
         dev_err(&pcard->pdev->dev, "probe_core_uio failed uio_register_device: %d\n", rv);
+        put_device(kudev->dev);
+        kfree(kudev);
         return rv;
     }
     
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 5/5] staging: kpc2000: clean up after probe failure.

[Jeremy Sowden]

On error, kp2000_probe_cores just returned an error without freeing
resources which had previously been allocated.  Added the missing
clean-up code.

Updated TODO.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/staging/kpc2000/TODO                 | 1 -
 drivers/staging/kpc2000/kpc2000/cell_probe.c | 9 +++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/kpc2000/TODO b/drivers/staging/kpc2000/TODO
index ed951acc829a..669fe5bf9637 100644
--- a/drivers/staging/kpc2000/TODO
+++ b/drivers/staging/kpc2000/TODO
@@ -1,7 +1,6 @@
 - the kpc_spi driver doesn't seem to let multiple transactions (to different instances of the core) happen in parallel...
 - The kpc_i2c driver is a hot mess, it should probably be cleaned up a ton.  It functions against current hardware though.
 - pcard->card_num in kp2000_pcie_probe() is a global variable and needs atomic / locking / something better.
-- the loop in kp2000_probe_cores() that uses probe_core_uio() also probably needs error handling
 - would be nice if the AIO fileops in kpc_dma could be made to work
     - probably want to add a CONFIG_ option to control compilation of the AIO functions
 - if the AIO fileops in kpc_dma start working, next would be making iov_count > 1 work too
diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
index e4c21291fe16..bce2bf9eee04 100644
--- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
+++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
@@ -438,7 +438,7 @@ int  kp2000_probe_cores(struct kp2000_device *pcard)
                 dev_err(&pcard->pdev->dev,
                         "kp2000_probe_cores: failed to add core %d: %d\n",
                         i, err);
-                return err;
+                goto error;
             }
             core_num++;
         }
@@ -457,10 +457,15 @@ int  kp2000_probe_cores(struct kp2000_device *pcard)
     err = probe_core_uio(0, pcard, "kpc_uio", cte);
     if (err){
         dev_err(&pcard->pdev->dev, "kp2000_probe_cores: failed to add board_info core: %d\n", err);
-        return err;
+        goto error;
     }
     
     return 0;
+
+error:
+    kp2000_remove_cores(pcard);
+    mfd_remove_devices(PCARD_TO_DEV(pcard));
+    return err;
 }
 
 void  kp2000_remove_cores(struct kp2000_device *pcard)
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] staging: kpc2000: added designated initializers to two structs.

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 1846 bytes --]

On 2019-05-15, at 14:11:53 +0300, Dan Carpenter wrote:
> On Wed, May 15, 2019 at 02:09:49PM +0300, Dan Carpenter wrote:
> > On Wed, May 15, 2019 at 11:34:52AM +0100, Jeremy Sowden wrote:
> > > diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > index 30e6f176ddfa..9cb745f4323a 100644
> > > --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > @@ -94,7 +94,7 @@ void parse_core_table_entry(struct core_table_entry *cte, const u64 read_val, co
> > >  static int probe_core_basic(unsigned int core_num, struct kp2000_device *pcard,
> > >  			    char *name, const struct core_table_entry cte)
> > >  {
> > > -    struct mfd_cell  cell = {0};
> > > +    struct mfd_cell  cell = { .id = core_num, .name = name };
> > >      struct resource  resources[2];
> > >
> > >      struct kpc_core_device_platdata  core_pdata = {
> > > @@ -315,7 +315,7 @@ static int probe_core_uio(unsigned int core_num, struct kp2000_device *pcard,
> > >
> > >  static int  create_dma_engine_core(struct kp2000_device *pcard, size_t engine_regs_offset, int engine_num, int irq_num)
> > >  {
> > > -    struct mfd_cell  cell = {0};
> > > +    struct mfd_cell  cell = { .id = engine_num };
> > >      struct resource  resources[2];
> > >
> >
> > These changes make no sense because we just write over it later.
> >
> > Maybe you're going to fix it up later in the patch series, perhaps
> > but that's not how it's done.  Each patch should do "one thing", not
> > "half and thing and then half a thing later in the series possibly
> > (I am reviewing the patches in order so I don't know)".
> >
>
> I've finished reviewing the series and we never complete the other
> half of this patch.

Stupid mistake.  I've sent out v2.

Thanks for the review.

J.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 0/5] staging: kpc2000: assorted fixes

[Dan Carpenter]

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/5] staging: kpc2000: inverted conditional in order to reduce indentation.

[Greg KH]

On Wed, May 15, 2019 at 12:14:33PM +0100, Jeremy Sowden wrote:
> Changed:
> 
>   for (...) {
>     ...
>     if (expr) {
>       ...
>     }
>   }
> 
> into:
> 
>   for (...) {
>     ...
>     if (!expr)
>       continue;
>     ...
>   }
> 
> in order to reduce indentation of conditional block.  Fixed indentation
> of cases blocks at the same time.
> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/staging/kpc2000/kpc2000/cell_probe.c | 43 +++++++++++---------
>  1 file changed, 24 insertions(+), 19 deletions(-)

Always be sure to cc: the proper maintainer and developers for your
patches.  Otherwise they might get lost in the noise of a mailing
list...
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/5] staging: kpc2000: inverted conditional in order to reduce indentation.

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 868 bytes --]

On 2019-05-15, at 15:14:51 +0200, Greg KH wrote:
> On Wed, May 15, 2019 at 12:14:33PM +0100, Jeremy Sowden wrote:
> > Changed:
> >
> >   for (...) {
> >     ...
> >     if (expr) {
> >       ...
> >     }
> >   }
> >
> > into:
> >
> >   for (...) {
> >     ...
> >     if (!expr)
> >       continue;
> >     ...
> >   }
> >
> > in order to reduce indentation of conditional block.  Fixed
> > indentation of cases blocks at the same time.
> >
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> >  drivers/staging/kpc2000/kpc2000/cell_probe.c | 43 +++++++++++---------
> >  1 file changed, 24 insertions(+), 19 deletions(-)
>
> Always be sure to cc: the proper maintainer and developers for your
> patches.  Otherwise they might get lost in the noise of a mailing
> list...

Understood.

Thanks,

J.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel