Re: [PATCH] soc: fsl: guts: us devm_kstrdup_const() for RO data

[Nicholas Mc Guire]

On Thu, Jan 10, 2019 at 01:43:01PM -0600, Li Yang wrote:
> On Sat, Dec 22, 2018 at 2:02 AM Nicholas Mc Guire  wrote:
> >
> > On Fri, Dec 21, 2018 at 08:29:56PM -0600, Scott Wood wrote:
> > > On Fri, 2018-12-07 at 09:22 +0100, Nicholas Mc Guire wrote:
> > > > devm_kstrdup() may return NULL if internal allocation failed, but
> > > > as  machine  is from the device tree, and thus RO, devm_kstrdup_const()
> > > > can be used here, which will only copy the reference.
> > >
> > > Is it really going to only copy the reference?  That would require that
> > > is_kernel_rodata(machine) be true, which it shouldn't be since it's not 
> > > part
> > > of the kernel image.
> > >
> > I had tried to figure out what is RO and what not but was not
> > able to determine that - from the discussion it seemed that the
> > assumption of RO is correct though I did not ask if it would
> > satisfy is_kernel_rodata() so that explains the incorrect assertion.
> > see https://lkml.org/lkml/2018/12/6/42
> > So then the only option is to check the return and cleanup
> > on allocation failure as the orriginal patch proposed.
> 
> Thanks for the good discussion. I will drop the previous patch. But
> would it also be good to just have "soc_dev_attr.machine = machine"
> directly?
>
I think that the intent is to switch to 
managed devm API so that the cleanup is handled properly
currently you would get "machine" from 
 of_property_read_string_index 
  -> of_property_read_string_helper
   -> of_find_property
which does not do any allocation - so there would actually
not be anything to cleanup here - don´t see why your solution
would not be suitable given the current API. the only advantage
of the devm_kstrdup() is that underlying APIs internal changes
would have no effect.

thx!
hofrat

Re: [PATCH] soc: fsl: guts: us devm_kstrdup_const() for RO data

[Nicholas Mc Guire]

On Fri, Dec 21, 2018 at 08:29:56PM -0600, Scott Wood wrote:
> On Fri, 2018-12-07 at 09:22 +0100, Nicholas Mc Guire wrote:
> > devm_kstrdup() may return NULL if internal allocation failed, but
> > as  machine  is from the device tree, and thus RO, devm_kstrdup_const()
> > can be used here, which will only copy the reference.
> 
> Is it really going to only copy the reference?  That would require that
> is_kernel_rodata(machine) be true, which it shouldn't be since it's not part
> of the kernel image.
>
I had tried to figure out what is RO and what not but was not 
able to determine that - from the discussion it seemed that the
assumption of RO is correct though I did not ask if it would
satisfy is_kernel_rodata() so that explains the incorrect assertion.
see https://lkml.org/lkml/2018/12/6/42
So then the only option is to check the return and cleanup
on allocation failure as the orriginal patch proposed.

thanks for clearifying this !
hofrat

[PATCH] soc: fsl: guts: us devm_kstrdup_const() for RO data

[Nicholas Mc Guire]

devm_kstrdup() may return NULL if internal allocation failed, but
as  machine  is from the device tree, and thus RO, devm_kstrdup_const()
can be used here, which will only copy the reference.

Signed-off-by: Nicholas Mc Guire 
Fixes: a6fc3b698130 ("soc: fsl: add GUTS driver for QorIQ platforms")
---

Problem located by experimental coccinelle script

Patch was compile tested with: multi_v7_defconfig (implies FSL_GUTS=y)

Patch is against 4.20-rc5 (localversion-next is next-20181207)

 drivers/soc/fsl/guts.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index 302e0c8..15071ec 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -157,7 +157,8 @@ static int fsl_guts_probe(struct platform_device *pdev)
of_property_read_string_index(root, "compatible", 0, );
of_node_put(root);
if (machine)
-   soc_dev_attr.machine = devm_kstrdup(dev, machine, GFP_KERNEL);
+   soc_dev_attr.machine = devm_kstrdup_const(dev, machine,
+ GFP_KERNEL);
 
svr = fsl_guts_get_svr();
soc_die = fsl_soc_die_match(svr, fsl_soc_die);
-- 
2.1.4

Re: [RFC PATCH] soc: fsl: guts: handle devm_kstrdup() failure

[Nicholas Mc Guire]

On Wed, Dec 05, 2018 at 02:42:55PM -0600, Li Yang wrote:
> On Sun, Dec 2, 2018 at 3:07 AM Nicholas Mc Guire  wrote:
> >
> > devm_kstrdup() may return NULL if internal allocation failed.
> > soc_dev_attr.machine  should be checked (although its only use
> > in pr_info() would be safe even with a NULL). Therefor
> > in the unlikely case of allocation failure, fsl_guts_probe() returns
> > -ENOMEM as this allocating failing is an indication of something
> > more serious going wrong at system level.
> >
> > As  machine  is from the device tree which I assume to be RO - if
> > that assumption is always correct - a better alternative would be
> > to use devm_kstrdup_const() here. That would then simply copy the
> > reference to the RO data and not perform any allocation at all.
> 
> I think your assumption is correct.  Do you want to send a new and
> better version?  :)

The issue was actually more general that I did not find a reliable
method to assure that some object is *always* RO. Even for device
tree data it was not clear to me if there could be systems where
it is not RO.
Anyway - will send the version using devm_kstrdup_const() then.

thx!
hofrat

> 
> >
> > Signed-off-by: Nicholas Mc Guire 
> > Fixes: a6fc3b698130 ("soc: fsl: add GUTS driver for QorIQ platforms")
> > ---
> >
> > Problem located by experimental coccinelle script
> >
> > Patch was compile tested with: multi_v7_defconfig (implies FSL_GUTS=y)
> >
> > Patch is against 4.20-rc4 (localversion-next is next-20181130)
> >
> >  drivers/soc/fsl/guts.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
> > index 302e0c8..a0c751b 100644
> > --- a/drivers/soc/fsl/guts.c
> > +++ b/drivers/soc/fsl/guts.c
> > @@ -156,8 +156,11 @@ static int fsl_guts_probe(struct platform_device *pdev)
> > if (of_property_read_string(root, "model", ))
> > of_property_read_string_index(root, "compatible", 0, 
> > );
> > of_node_put(root);
> > -   if (machine)
> > +   if (machine) {
> > soc_dev_attr.machine = devm_kstrdup(dev, machine, 
> > GFP_KERNEL);
> > +   if (!soc_dev_attr.machine)
> > +   return -ENOMEM;
> > +   }
> >
> > svr = fsl_guts_get_svr();
> > soc_die = fsl_soc_die_match(svr, fsl_soc_die);
> > --
> > 2.1.4
> >

[RFC PATCH] soc: fsl: guts: handle devm_kstrdup() failure

[Nicholas Mc Guire]

devm_kstrdup() may return NULL if internal allocation failed.
soc_dev_attr.machine  should be checked (although its only use
in pr_info() would be safe even with a NULL). Therefor
in the unlikely case of allocation failure, fsl_guts_probe() returns
-ENOMEM as this allocating failing is an indication of something
more serious going wrong at system level.

As  machine  is from the device tree which I assume to be RO - if
that assumption is always correct - a better alternative would be
to use devm_kstrdup_const() here. That would then simply copy the
reference to the RO data and not perform any allocation at all.

Signed-off-by: Nicholas Mc Guire 
Fixes: a6fc3b698130 ("soc: fsl: add GUTS driver for QorIQ platforms")
---

Problem located by experimental coccinelle script

Patch was compile tested with: multi_v7_defconfig (implies FSL_GUTS=y)

Patch is against 4.20-rc4 (localversion-next is next-20181130)

 drivers/soc/fsl/guts.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/fsl/guts.c b/drivers/soc/fsl/guts.c
index 302e0c8..a0c751b 100644
--- a/drivers/soc/fsl/guts.c
+++ b/drivers/soc/fsl/guts.c
@@ -156,8 +156,11 @@ static int fsl_guts_probe(struct platform_device *pdev)
if (of_property_read_string(root, "model", ))
of_property_read_string_index(root, "compatible", 0, );
of_node_put(root);
-   if (machine)
+   if (machine) {
soc_dev_attr.machine = devm_kstrdup(dev, machine, GFP_KERNEL);
+   if (!soc_dev_attr.machine)
+   return -ENOMEM;
+   }
 
svr = fsl_guts_get_svr();
soc_die = fsl_soc_die_match(svr, fsl_soc_die);
-- 
2.1.4

[PATCH 1/2] usb: gadget: fsl_udc_core: check allocation return value and cleanup on failure

[Nicholas Mc Guire]

The allocation with fsl_alloc_request() and kmalloc() were unchecked
fixed this up with a NULL check and appropriate cleanup.

Additionally udc->ep_qh_size was reset to 0 on failure of allocation.
Similar udc->phy_mode is initially 0 (as udc_controller was
allocated with kzalloc in fsl_udc_probe()) so reset it to 0 as well
so that this function is side-effect free on failure. Not clear if
this is necessary or sensible as fsl_udc_release() probably can not
be called if fsl_udc_probe() failed - but it should not hurt.

Signed-off-by: Nicholas Mc Guire 
Fixes: b504882da5 ("USB: add Freescale high-speed USB SOC device controller 
driver")
---

Problem located with experimental coccinelle script

Patch was compile tested with: imx_v6_v7_defconfig (implies USB_FSL_USB2=y)
(with a large number of sparse warnings not related to the proposed change
 and one smatch warning)

Patch is against 4.19-rc1 (localversion-next is next-20180830)

 drivers/usb/gadget/udc/fsl_udc_core.c | 30 ++
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c 
b/drivers/usb/gadget/udc/fsl_udc_core.c
index be59309..e637afb 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -2247,8 +2247,10 @@ static int struct_udc_setup(struct fsl_udc *udc,
udc->phy_mode = pdata->phy_mode;
 
udc->eps = kcalloc(udc->max_ep, sizeof(struct fsl_ep), GFP_KERNEL);
-   if (!udc->eps)
-   return -1;
+   if (!udc->eps) {
+   ERR("kmalloc udc endpoint status failed\n");
+   goto eps_alloc_failed;
+   }
 
/* initialized QHs, take care of alignment */
size = udc->max_ep * sizeof(struct ep_queue_head);
@@ -2262,8 +2264,7 @@ static int struct_udc_setup(struct fsl_udc *udc,
>ep_qh_dma, GFP_KERNEL);
if (!udc->ep_qh) {
ERR("malloc QHs for udc failed\n");
-   kfree(udc->eps);
-   return -1;
+   goto ep_queue_alloc_failed;
}
 
udc->ep_qh_size = size;
@@ -2272,8 +2273,17 @@ static int struct_udc_setup(struct fsl_udc *udc,
/* FIXME: fsl_alloc_request() ignores ep argument */
udc->status_req = container_of(fsl_alloc_request(NULL, GFP_KERNEL),
struct fsl_req, req);
+   if (!udc->status_req) {
+   ERR("kzalloc for udc status request failed\n");
+   goto udc_status_alloc_failed;
+   }
+
/* allocate a small amount of memory to get valid address */
udc->status_req->req.buf = kmalloc(8, GFP_KERNEL);
+   if (!udc->status_req->req.buf) {
+   ERR("kzalloc for udc request buffer failed\n");
+   goto udc_req_buf_alloc_failed;
+   }
 
udc->resume_state = USB_STATE_NOTATTACHED;
udc->usb_state = USB_STATE_POWERED;
@@ -2281,6 +2291,18 @@ static int struct_udc_setup(struct fsl_udc *udc,
udc->remote_wakeup = 0; /* default to 0 on reset */
 
return 0;
+
+udc_req_buf_alloc_failed:
+   kfree(udc->status_req);
+udc_status_alloc_failed:
+   kfree(udc->ep_qh);
+   udc->ep_qh_size = 0;
+ep_queue_alloc_failed:
+   kfree(udc->eps);
+eps_alloc_failed:
+   udc->phy_mode = 0;
+   return -1;
+
 }
 
 /*
-- 
2.1.4

[PATCH 2/2] usb: gadget: fsl_udc_core: fixup struct_udc_setup documentation

[Nicholas Mc Guire]

The original implementation from commit b504882da539
("USB: add Freescale high-speed USB SOC device controller driver")
returned NULL on failure and an allocated + initialized struct fsl_udc on
success. The current code introduced in commit 4365831dadfe
("USB: fsl_usb2_udc: Get max ep number from DCCPARAMS register") only
provides partial initialization as well as returning 0 on success and -1
on failures. The function documentation is updated accordingly.

Signed-off-by: Nicholas Mc Guire 
Fixes: 4365831dadfe ("USB: fsl_usb2_udc: Get max ep number from DCCPARAMS 
register")
---

Problem located during code-review

Patch is against 4.19-rc1 (localversion-next is next-20180830)

 drivers/usb/gadget/udc/fsl_udc_core.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c 
b/drivers/usb/gadget/udc/fsl_udc_core.c
index e637afb..680b46e 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -2234,8 +2234,10 @@ static void fsl_udc_release(struct device *dev)
Internal structure setup functions
 ***/
 /*--
- * init resource for globle controller
- * Return the udc handle on success or NULL on failure
+ * init resource for global controller called by fsl_udc_probe()
+ * On success the udc handle is initialized, on failure it is
+ * unchanged (reset).
+ * Return 0 on success and -1 on allocation failure
  --*/
 static int struct_udc_setup(struct fsl_udc *udc,
struct platform_device *pdev)
-- 
2.1.4

[PATCH] KVM: PPC: Book3S HV: fix constant size warning

[Nicholas Mc Guire]

 The constants are 64bit but not explicitly declared UL resulting
in sparse warnings. Fixed by declaring the constants UL.

Signed-off-by: Nicholas Mc Guire 
---

sparse fallout from compile checking book3s_hv.c:
arch/powerpc/kvm/book3s_hv.c:141:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:142:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:143:9: warning: constant 0x7FFF2908450D8DA9 is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:144:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:145:9: warning: constant 0x199A421245058DA9 is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:146:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:147:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:148:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:149:9: warning: constant 0x164520C62609AECA is so 
big it is long
arch/powerpc/kvm/book3s_hv.c:1667:60: warning: constant 0xf3ff is 
so big it is unsigned long
arch/powerpc/kvm/book3s_hv.c:2896:42: warning: constant 0x164520C62609AECA is 
so big it is long

most of the warnings are local constants in book3s_hv.c, PSSCR_GUEST_VIS
from reg.h is not. Although PSSCR_GUEST_VIS is not #ifdef CONFIG_PPC_BOOK3S
it seems only to be used in that particular platform (check with cscope).

The constants fit in the target variables so the size declaration 
discrepancy does not actually cause a problem here.

Patch was compiletested with: ppc64_defconfig (implies
CONFIG_KVM=y, CONFIG_KVM_BOOK3S_64_HV=m)

Patch is against 4.18-rc3 (localversion-next is next-20180705)

 arch/powerpc/include/asm/reg.h |  2 +-
 arch/powerpc/kvm/book3s_hv.c   | 16 
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 5625684..858aa79 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -161,7 +161,7 @@
 #define PSSCR_ESL  0x0020 /* Enable State Loss */
 #define PSSCR_SD   0x0040 /* Status Disable */
 #define PSSCR_PLS  0xf000 /* Power-saving Level Status */
-#define PSSCR_GUEST_VIS0xf3ff /* Guest-visible PSSCR 
fields */
+#define PSSCR_GUEST_VIS0xf3ffUL /* Guest-visible PSSCR 
fields */
 #define PSSCR_FAKE_SUSPEND 0x0400 /* Fake-suspend bit (P9 DD2.2) */
 #define PSSCR_FAKE_SUSPEND_LG  10 /* Fake-suspend bit position */
 
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index ee4a885..c517859 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -128,14 +128,14 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu 
*vcpu);
  * and SPURR count and should be set according to the number of
  * online threads in the vcore being run.
  */
-#define RWMR_RPA_P8_1THREAD0x164520C62609AECA
-#define RWMR_RPA_P8_2THREAD0x7FFF2908450D8DA9
-#define RWMR_RPA_P8_3THREAD0x164520C62609AECA
-#define RWMR_RPA_P8_4THREAD0x199A421245058DA9
-#define RWMR_RPA_P8_5THREAD0x164520C62609AECA
-#define RWMR_RPA_P8_6THREAD0x164520C62609AECA
-#define RWMR_RPA_P8_7THREAD0x164520C62609AECA
-#define RWMR_RPA_P8_8THREAD0x164520C62609AECA
+#define RWMR_RPA_P8_1THREAD0x164520C62609AECAUL
+#define RWMR_RPA_P8_2THREAD0x7FFF2908450D8DA9UL
+#define RWMR_RPA_P8_3THREAD0x164520C62609AECAUL
+#define RWMR_RPA_P8_4THREAD0x199A421245058DA9UL
+#define RWMR_RPA_P8_5THREAD0x164520C62609AECAUL
+#define RWMR_RPA_P8_6THREAD0x164520C62609AECAUL
+#define RWMR_RPA_P8_7THREAD0x164520C62609AECAUL
+#define RWMR_RPA_P8_8THREAD0x164520C62609AECAUL
 
 static unsigned long p8_rwmr_values[MAX_SMT_THREADS + 1] = {
RWMR_RPA_P8_1THREAD,
-- 
2.1.4

[PATCH] KVM: PPC: Book3S HV: add of_node_put() in success path

[Nicholas Mc Guire]

 The call to of_find_compatible_node() is returning a pointer with
incremented refcount so it must be explicitly decremented after the
last use. As here it is only being used for checking of node presence
but the result is not actually used in the success path it can be
dropped immediately.

Signed-off-by: Nicholas Mc Guire 
Fixes: commit f725758b899f ("KVM: PPC: Book3S HV: Use OPAL XICS emulation on 
POWER9")
---
Problem found by experimental coccinelle script

Patch was compiletested with: ppc64_defconfig (implies CONFIG_KVM=y,
CONFIG_KVM_BOOK3S_64_HV=m)
with many sparse warnings though not related to the proposed change

Patch is against 4.18-rc3 (localversion-next is next-20180705)

 arch/powerpc/kvm/book3s_hv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index ee4a885..8680fb9 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -4561,6 +4561,8 @@ static int kvmppc_book3s_init_hv(void)
pr_err("KVM-HV: Cannot determine method for accessing 
XICS\n");
return -ENODEV;
}
+   /* presence of intc confirmed - node can be dropped again */
+   of_node_put(np);
}
 #endif
 
-- 
2.1.4

[PATCH] powerpc: icp-hv: fix missing of_node_put in success path

[Nicholas Mc Guire]

 Both of_find_compatible_node() and of_find_node_by_type() will
return a refcounted node on success - thus for the success path
the node must be explicitly released with a of_node_put().

Signed-off-by: Nicholas Mc Guire 
Fixes: commit 0b05ac6e2480 ("powerpc/xics: Rewrite XICS driver")
---
Problem found by experimental coccinelle script

Patch was compiletested with: ppc64_defconfig (implies
CONFIG_PPC_ICP_HV=y)
with sparse warnings though not related to the proposed change

Patch is against 4.18-rc3 (localversion-next is next-20180704)

 arch/powerpc/sysdev/xics/icp-hv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/xics/icp-hv.c 
b/arch/powerpc/sysdev/xics/icp-hv.c
index bbc839a..003deaa 100644
--- a/arch/powerpc/sysdev/xics/icp-hv.c
+++ b/arch/powerpc/sysdev/xics/icp-hv.c
@@ -179,6 +179,7 @@ int icp_hv_init(void)
 
icp_ops = _hv_ops;
 
+   of_node_put(np);
return 0;
 }
 
-- 
2.1.4

[PATCH] powerpc: hwrng; fix missing of_node_put()

[Nicholas Mc Guire]

 The call to of_find_compatible_node() returns a node pointer with refcount
incremented thus it must be explicitly decremented here before returning.

Signed-off-by: Nicholas Mc Guire 
Fixes: commit a489043f4626 ("powerpc/pseries: Implement arch_get_random_long() 
based on H_RANDOM")
---
Problem found with experimental coccinelle script

Patch was compiletested with: ppc64_defconfig (implies CONFIG_PPC_PSERIES=y)
with some unrelated sparse warnings (which I did not understand)
./arch/powerpc/include/asm/head-64.h:13:36: warning: Unknown escape '('
./arch/powerpc/include/asm/head-64.h:16:36: warning: Unknown escape '('
./arch/powerpc/include/asm/head-64.h:19:36: warning: Unknown escape '('

Patch is aginst 4.18-rc2 (localversion-next is next-20180702)

 arch/powerpc/platforms/pseries/rng.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/platforms/pseries/rng.c 
b/arch/powerpc/platforms/pseries/rng.c
index 31ca557..262b8c5 100644
--- a/arch/powerpc/platforms/pseries/rng.c
+++ b/arch/powerpc/platforms/pseries/rng.c
@@ -40,6 +40,7 @@ static __init int rng_init(void)
 
ppc_md.get_random_seed = pseries_get_random_long;
 
+   of_node_put(dn);
return 0;
 }
 machine_subsys_initcall(pseries, rng_init);
-- 
2.1.4

[PATCH] ALSA: snd-aoa: add of_node_put() in error path

[Nicholas Mc Guire]

 Both calls to of_find_node_by_name() and of_get_next_child() return a
node pointer with refcount incremented thus it must be explicidly
decremented here after the last usage. As we are assured to have a
refcounted  np  either from the initial
of_find_node_by_name(NULL, name); or from the of_get_next_child(gpio, np)
in the while loop if we reached the error code path below, an
x of_node_put(np) is needed.

Signed-off-by: Nicholas Mc Guire 
Fixes: commit f3d9478b2ce4 ("[ALSA] snd-aoa: add snd-aoa")
---

Problem located by an experimental coccinelle script

Patch was compiletested with: ppc64_defconfig (implies CONFIG_SND_AOA=m)

Patch is against 4.18-rc2 (localversion-next is next-20180629)

 sound/aoa/core/gpio-feature.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/aoa/core/gpio-feature.c b/sound/aoa/core/gpio-feature.c
index 7196008..6555742 100644
--- a/sound/aoa/core/gpio-feature.c
+++ b/sound/aoa/core/gpio-feature.c
@@ -88,8 +88,10 @@ static struct device_node *get_gpio(char *name,
}
 
reg = of_get_property(np, "reg", NULL);
-   if (!reg)
+   if (!reg) {
+   of_node_put(np);
return NULL;
+   }
 
*gpioptr = *reg;
 
-- 
2.1.4

Re: linux-next: build warnings after merge of the kbuild tree

[Nicholas Mc Guire]

On Fri, Aug 26, 2016 at 01:58:03PM +1000, Nicholas Piggin wrote:
> On Mon, 22 Aug 2016 20:47:58 +1000
> Nicholas Piggin  wrote:
> 
> > On Fri, 19 Aug 2016 20:44:55 +1000
> > Nicholas Piggin  wrote:
> > 
> > > On Fri, 19 Aug 2016 10:37:00 +0200
> > > Michal Marek  wrote:
> > >   
> > > > On 2016-08-19 07:09, Stephen Rothwell wrote:
> > 
> > [snip]
> > 
> > > > > 
> > > > > I may be missing something, but genksyms generates the crc's off the
> > > > > preprocessed C source code and we don't have any for the asm files 
> > > > > ...  
> > > > 
> > > > Of course you are right. Which means that we are losing type information
> > > > for these exports for CONFIG_MODVERSIONS purposes. I guess it's
> > > > acceptable, since the asm functions are pretty basic and their
> > > > signatures do not change.
> > > 
> > > I don't completely agree. It would be nice to have the functionality
> > > still there.
> > > 
> > > What happens if you just run cmd_modversions on the as rule? It relies on
> > > !defined(__ASSEMBLY__), but we're feeding the result to genksyms, not as.
> > > It would require the header be included in the .S file and be protected 
> > > for
> > > asm builds.  
> > 
> > 
> > This seems like it *could* be made to work, but there's a few problems.
> > 
> > - .h files are not made for C consumption. Matter of manually adding the
> > ifdef guards, which isn't terrible.
> > 
> > - .S files do not all include their .h where the C declaration is. Also
> > will cause some churn but doable and maybe not completely unreasonable.
> > 
> > - genksyms parser barfs when it hits the assembly of the .S file. Best
> > way to fix that seems just send the #include and EXPORT_SYMBOL lines
> > from the .S to the preprocessor. That's a bit of a rabbit hole too, with
> > some .S files being included, etc.
> > 
> > I'm not sure what to do here. If nobody cares and we lose CRCs for .S
> > exports, then okay we can whitelist those relocs easily. If we don't want
> > to lose the functionality, the above might work but it's a bit intrusive
> > an is going to require another cycle of prep patches to go through arch
> > code first.
> > 
> > Or suggestions for alternative approach?
> 
> Here is a quick patch that I think should catch missing CRCs in
> architecture independent way. If we merge something like this, we
> can whitelist the symbols in arch/powerpc so people get steered to
> the right place.
> 
> Powerpc seems to be the only one really catching this, and it's
> only as a side effect of a test run for CONFIG_RELOCATABLE kernels,
> which means version failures probably slipped through other archs.
> 
> I'll clean it up, do some more testing, and submit it unless
> anybody dislikes it or has a better way to do it.
> 
> Thanks,
> Nick
> 
> 
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 4b8ffd3..1efc454 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -609,6 +609,7 @@ static void handle_modversions(struct module *mod, struct 
> elf_info *info,
>  {
>   unsigned int crc;
>   enum export export;
> + int is_crc = 0;

should that not be a bool here ?

>  
>   if ((!is_vmlinux(mod->name) || mod->is_dot_o) &&
>   strncmp(symname, "__ksymtab", 9) == 0)
> @@ -618,6 +619,7 @@ static void handle_modversions(struct module *mod, struct 
> elf_info *info,
>  
>   /* CRC'd symbol */
>   if (strncmp(symname, CRC_PFX, strlen(CRC_PFX)) == 0) {
> + is_crc = 1;

is_crc = true;

>   crc = (unsigned int) sym->st_value;
>   sym_update_crc(symname + strlen(CRC_PFX), mod, crc,
>   export);

thx!
hofrat

[PATCH] macintosh: ams: cleanup on stack DECLARE_COMPLETION

[Nicholas Mc Guire]

fix-up for incorrect use of DECLARE_COMPLETION. see also commit
6e9a4738 (completions: lockdep annotate on stack completions)

V2: split out patch for individual files and (hopefully) proper
labeling this time

patch is against linux-next 3.19.0-rc1 -next-20141226

patch was compile tested with ppc6xx_defconfig, CONFIG_SENSORS_AMS=m,
CONFIG_SENSORS_AMS_PMU=y

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---
 drivers/macintosh/ams/ams-pmu.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/macintosh/ams/ams-pmu.c b/drivers/macintosh/ams/ams-pmu.c
index 4f61b3e..c2b178f 100644
--- a/drivers/macintosh/ams/ams-pmu.c
+++ b/drivers/macintosh/ams/ams-pmu.c
@@ -52,7 +52,7 @@ static void ams_pmu_req_complete(struct adb_request *req)
 static void ams_pmu_set_register(u8 reg, u8 value)
 {
static struct adb_request req;
-   DECLARE_COMPLETION(req_complete);
+   DECLARE_COMPLETION_ONSTACK(req_complete);
 
req.arg = req_complete;
if (pmu_request(req, ams_pmu_req_complete, 4, ams_pmu_cmd, 0x00, reg, 
value))
@@ -65,7 +65,7 @@ static void ams_pmu_set_register(u8 reg, u8 value)
 static u8 ams_pmu_get_register(u8 reg)
 {
static struct adb_request req;
-   DECLARE_COMPLETION(req_complete);
+   DECLARE_COMPLETION_ONSTACK(req_complete);
 
req.arg = req_complete;
if (pmu_request(req, ams_pmu_req_complete, 3, ams_pmu_cmd, 0x01, reg))
-- 
1.7.10.4

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] cleanup on stack DECLARE_COMPLETIONs

[Nicholas Mc Guire]

fixups for incorrect use of DECLARE_COMPLETION. see also commit
6e9a4738 (completions: lockdep annotate on stack completions)
The only somewhat special case being
drivers/misc/sgi-gru/grukservices.c:quicktest2
which had a static qualifier in the original DECLARE_COMPLETION()
but that seems to be wrong (why should the completion persisted between
successive calls ?) so the conversion to DECLARE_COMPLETION_ONSTACK
was also applied and the static qualifier removed.

Not sure if this is suitable in this form or if it should go out as
5 seperate patches ? 

This was only code reviewed and compile tested

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---
 drivers/macintosh/ams/ams-pmu.c   |4 ++--
 drivers/misc/sgi-gru/grukservices.c   |2 +-
 drivers/scsi/aha152x.c|2 +-
 drivers/usb/gadget/udc/fsl_qe_udc.c   |2 +-
 drivers/usb/gadget/udc/fsl_udc_core.c |2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/macintosh/ams/ams-pmu.c b/drivers/macintosh/ams/ams-pmu.c
index 4f61b3e..c2b178f 100644
--- a/drivers/macintosh/ams/ams-pmu.c
+++ b/drivers/macintosh/ams/ams-pmu.c
@@ -52,7 +52,7 @@ static void ams_pmu_req_complete(struct adb_request *req)
 static void ams_pmu_set_register(u8 reg, u8 value)
 {
static struct adb_request req;
-   DECLARE_COMPLETION(req_complete);
+   DECLARE_COMPLETION_ONSTACK(req_complete);

req.arg = req_complete;
if (pmu_request(req, ams_pmu_req_complete, 4, ams_pmu_cmd, 0x00, reg, 
value))
@@ -65,7 +65,7 @@ static void ams_pmu_set_register(u8 reg, u8 value)
 static u8 ams_pmu_get_register(u8 reg)
 {
static struct adb_request req;
-   DECLARE_COMPLETION(req_complete);
+   DECLARE_COMPLETION_ONSTACK(req_complete);

req.arg = req_complete;
if (pmu_request(req, ams_pmu_req_complete, 3, ams_pmu_cmd, 0x01, reg))
diff --git a/drivers/misc/sgi-gru/grukservices.c 
b/drivers/misc/sgi-gru/grukservices.c
index 913de07..4e412fe 100644
--- a/drivers/misc/sgi-gru/grukservices.c
+++ b/drivers/misc/sgi-gru/grukservices.c
@@ -1044,7 +1044,7 @@ done:

 static int quicktest2(unsigned long arg)
 {
-   static DECLARE_COMPLETION(cmp);
+   DECLARE_COMPLETION_ONSTACK(cmp);
unsigned long han;
int blade_id = 0;
int numcb = 4;
diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 2b960b3..b16afb9 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -1055,7 +1055,7 @@ static int aha152x_abort(Scsi_Cmnd *SCpnt)
 static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 {
struct Scsi_Host *shpnt = SCpnt-device-host;
-   DECLARE_COMPLETION(done);
+   DECLARE_COMPLETION_ONSTACK(done);
int ret, issued, disconnected;
unsigned char old_cmd_len = SCpnt-cmd_len;
unsigned long flags;
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c 
b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 795c99c..e0822f1 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -2630,7 +2630,7 @@ static int qe_udc_remove(struct platform_device *ofdev)
struct qe_udc *udc = platform_get_drvdata(ofdev);
struct qe_ep *ep;
unsigned int size;
-   DECLARE_COMPLETION(done);
+   DECLARE_COMPLETION_ONSTACK(done);

usb_del_gadget_udc(udc-gadget);

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c 
b/drivers/usb/gadget/udc/fsl_udc_core.c
index 2df8074..c3830ad 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -2529,7 +2529,7 @@ static int __exit fsl_udc_remove(struct platform_device 
*pdev)
struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
struct fsl_usb2_platform_data *pdata = dev_get_platdata(pdev-dev);

-   DECLARE_COMPLETION(done);
+   DECLARE_COMPLETION_ONSTACK(done);

if (!udc_controller)
return -ENODEV;
--
1.7.10.4

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH] gadget: cleanup on stack DECLARE_COMPLETIONs

[Nicholas Mc Guire]

fixups for incorrect use of DECLARE_COMPLETION. see also commit
6e9a4738 (completions: lockdep annotate on stack completions)

patch is against 3.18.0 linux-next

This was only code reviewed and compile tested

Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
---
 drivers/usb/gadget/udc/fsl_qe_udc.c   |2 +-
 drivers/usb/gadget/udc/fsl_udc_core.c |2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c 
b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 795c99c..e0822f1 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -2630,7 +2630,7 @@ static int qe_udc_remove(struct platform_device *ofdev)
struct qe_udc *udc = platform_get_drvdata(ofdev);
struct qe_ep *ep;
unsigned int size;
-   DECLARE_COMPLETION(done);
+   DECLARE_COMPLETION_ONSTACK(done);

usb_del_gadget_udc(udc-gadget);

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c 
b/drivers/usb/gadget/udc/fsl_udc_core.c
index 2df8074..c3830ad 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -2529,7 +2529,7 @@ static int __exit fsl_udc_remove(struct platform_device 
*pdev)
struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
struct fsl_usb2_platform_data *pdata = dev_get_platdata(pdev-dev);

-   DECLARE_COMPLETION(done);
+   DECLARE_COMPLETION_ONSTACK(done);

if (!udc_controller)
return -ENODEV;
--
1.7.10.4

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: How to build the kernel without any optimization?

[Nicholas Mc Guire]

On Fri, 20 Aug 2010, Shawn Jin wrote:

 Hi,
 
 I'm tracing the execution of ds1307_probe() and find that some of
 variables or function arguments cannot be printed in gdb because they
 are optimized out or not in the current context. This really gives
 some headache. Is there a way to build the kernel without any
 optimization? What gcc option shall I disable or add?
 
 I already added the following to arch/powerpc/Makefile.
 
 # Prevent GDB from jumping around in the code when trying to single step
 ifeq ($(CONFIG_DEBUG_KERNEL),y)
 KBUILD_CFLAGS   += -fno-schedule-insns -fno-schedule-insns2
 endif


much of the kernel can not be build without optimization - what you 
can do though is slectively try to disable optimization for specific
files by putting 

 CFLAGS_REMOVE_objfilenam.o = -SOME_OPT

in the Makefile. I think that is safer than what you did above as this would
always depend on the order of options that ultimately get passed to gcc.

hofrat 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: can the kernel show user task stack backtrace ?

[Nicholas Mc Guire]

On Thu, 30 Jul 2009, Alessandro Rubini wrote:

  We're dealing with some complex (3rd party) applications and I like to see a
  user task stack backtrace.
  
  (Of course the way to go here is to use a debugger (gdb) and
do a backtrace (with the coredump file).
 
 Actually, you can intercept SIGSEGV and print your own stack from within
 the signal handler. You can also open /proc/self/maps and print it, to
 ease understanding the various pointers in there, especially if the
 application is using a number of shared libs.
 
 This is usually easier than getting to a core dump, although there is
 less information than what the core offers.
 
 I have the code for ARM and I've it on ppc once, but I must dig for the actual
 code.

I think libSegFault.so (part of glibc) can do that by simply preloading it 

LD_PRELOAD=/lib/libSegFault.so ./your_segfaulting_app

should do the trick.

hofrat
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Memory usage of a process

[Nicholas Mc Guire]

On Fri, 10 Jul 2009, G?nter Leonhardt wrote:

  
 Hello,
 analysing the memory usage of a process, I found some questions.
 I'am using a system with 128 MB physical RAM, no disk, 2.6.27 kernel.
 
 Running top, I see 38 MB in use, 90 MB free, but a VSZ for my process of 158 
 MB.
 Looking at /proc/pid/maps, I see that the process uses 140 MB without 
 shared libs.
 
 So how it is possible that the process can allocate more memory than there 
 is, without posibility of swapping?

 Why said top that 90 MB free? Does the kernel serve the allocation only if 
 ist really used?
 

an allocation does not actually set aside any physical ram (unless you do an 
mlock and actually touch the memory) - it only reserves virtual memory areas 
(vma's) and when your process accesses the respective address the OS 
page-faults and prvides the physical RAM as needed (and possible) - if you have 
no swap and you allow overcommitting of memory then you are potentially going 
to see this application failing as soon as it tries to utilize the memory.

This can be set in the system via /proc/sys/vm/overcommit_memory (see
linux/Documentation/vm/overcommit-accounting for details).
 
hofrat
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Device node - How does kernel know about it

[Nicholas Mc Guire]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

 * Ramdisk is also executing fine, just that prints are not coming out of
 serial. I can see the execution of various user programs with a printk
 in sys_execve() routine. Ramdisk has all the required files like
 /dev/console, /dev/ttyS0, etc.
 * Looking further into tty driver, I noticed that call to tty_write() or
 do_tty_write() is not happening at all. So, somewhere the interface
 between kernel and user program is lost.
 * Just to check it out, I tried to write a small kernel module and a
 test program.
  - Attached memtest.c module (not really testing memory there. :-))
  - Attached testmemtest.c user program, that just open's it and reads
 the information
  - Created a device node using mknod /dev/memtest c 168 0
  - When I do insmod memtest.ko inside the ramdisk bootup scripts, I
 could see all the printk's on the console
  - When I execute testmemtest next in the same script, it does not
 display the printk inside of memtest.c module. This only indicates that
 read call did not really go to the kernel side.
  - Just to check my program's validity, I checked on a similar machine
 and all the code works fine.
  - uname -r also matches with what I built. So, chances of exiting
 from open call because of mismatch is remote. Since userland cannot
 print, I have no idea what exactly is happening there.

The kernel will simply look at the major:minor numbers - so maybe you
simply have a wrong major/minor for /dev/ttyS0 ? in that case you will
see nothing but other than that most things will go on working.

hofrat
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFHdLY2nU7rXZKfY2oRApFpAKCKfGanKHGuFFJmUFy3aQtjmWNjEACfU7uK
hrfpn2RMn5l23ZqCOXV5rd8=
=GfsF
-END PGP SIGNATURE-
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: rtlinux rtai interrupt latency

[Nicholas Mc Guire]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

 On Sun, 2007-10-21 at 10:45 +0800, Bai Shuwei wrote:
 all, hi
   does anyone knows RTLinux, RTAI interrupt latency and schedule
 latency on the PPC440, PPC405?

 You'd have to ask Wind River.

There are free variants around as well - XtratuM/PPC (RTLinux-4.0) is
running on 440EP/GR and 405 Octobus, im quite sur RTAI also is available
for AMCC CPUs - check RTAI.org (mailing list link is to be found there) 
and rtlinux-gpl.org (mailing list link) - those lists are most likely the
best source for the free-software hard-realtime Linux variants.

hofrat
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.2.4 (GNU/Linux)

iD4DBQFHGmZZnU7rXZKfY2oRArrjAJ9WXETFjALA52TRuRtDRSm4x3DcZACYgylW
60r4W0r6+LtMv+UBK+5h9A==
=Eb2V
-END PGP SIGNATURE-
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: ppc manual paging question

[Nicholas Mc Guire]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1


  I've got some qeustion about ppc(ppc44x) paging:

 how can I manually map a virtual address to a physical address through a
 specific pgd? How does ppc translate virt address to physical one? I think
 besides from tlb, the CPU will search the page table entries via the pgd, can
 I alter the pgd value to change the memory translation? under i386, it's very
 simple, we can just rewrite %%cr3, it even could invalidate all tlb entries
 automatically, how can I do this under ppc? I've tried rewrite
 current-mm-pgd and current-thread.pgdir, but sounds like it still not
 insufficiant, am I missing something vital?


sur ! same thing flush the tlb 
or you will be totally inconsistant - actually the box should not
have survived this treatment in a stable manner. You might want to
look at kernel/ppc-stub.c as a good reference - its a similar problem
gdb also writes into memory without the kernel knowing about it - that
is comparable to what XM is doing - the solution flush the cache/tlb
all over the place - now flushing the cache is evil - but I would do
it for now and we can check alternatives and optimizations later - for
now brute force is the way to go.

for 4xx dont forget to isync after fidling with pgd/pte (see set_context
in entry_4xx.S . As far as I understand 4xx all you would really need to
do is clear the TLB entry and then you get an exception and that is
handled via 0x1100/0x1200 D/I respectively (head_4xx.S) - admitedly Im
not up to this task - need to give the manuals a lock my self to 
understand whats going on there ;)

hofrat
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFHHCmpnU7rXZKfY2oRAjLmAJ90QwCBHLaglOfJ5QAnJyCCIZDmGwCgh/fD
E76Ki1FdfofUSuVBXL1tG0M=
=/1C5
-END PGP SIGNATURE-
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev