[PATCH] drivers: of: removing assignment of 0 to static variable

[PATCH] drivers: of: removing assignment of 0 to static variable

[Saurabh Sengar]

no need to initialise static variable with 0, hence correcting it.

Signed-off-by: Saurabh Sengar <saurabh.truth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6e82bc42..63699dd 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -184,7 +184,7 @@ static void * unflatten_dt_node(const void *blob,
 	struct property *pp, **prev_pp = NULL;
 	const char *pathp;
 	unsigned int l, allocl;
-	static int depth = 0;
+	static int depth;
 	int old_depth;
 	int offset;
 	int has_name = 0;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drivers: of: removing assignment of 0 to static variable

[Saurabh Sengar]

no need to initialise static variable with 0, hence correcting it.

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
 drivers/of/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6e82bc42..63699dd 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -184,7 +184,7 @@ static void * unflatten_dt_node(const void *blob,
 	struct property *pp, **prev_pp = NULL;
 	const char *pathp;
 	unsigned int l, allocl;
-	static int depth = 0;
+	static int depth;
 	int old_depth;
 	int offset;
 	int has_name = 0;
-- 
1.9.1

Re: [PATCH] drivers: of: removing assignment of 0 to static variable

[Rob Herring]

On Mon, Oct 26, 2015 at 10:42 PM, Saurabh Sengar
<saurabh.truth@gmail.com> wrote:
> no need to initialise static variable with 0, hence correcting it.
>
> Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>

Applied. Thanks.

Rob

> ---
>  drivers/of/fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 6e82bc42..63699dd 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -184,7 +184,7 @@ static void * unflatten_dt_node(const void *blob,
>         struct property *pp, **prev_pp = NULL;
>         const char *pathp;
>         unsigned int l, allocl;
> -       static int depth = 0;
> +       static int depth;
>         int old_depth;
>         int offset;
>         int has_name = 0;
> --
> 1.9.1
>

[PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
should be atomic
GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
fail but certainly avoids deadlock

Signed-off-by: Saurabh Sengar <saurabh.truth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/core/sa_query.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 8c014b3..cd1f911 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -526,7 +526,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
 	if (len <= 0)
 		return -EMSGSIZE;
 
-	skb = nlmsg_new(len, GFP_KERNEL);
+	skb = nlmsg_new(len, GFP_ATOMIC);
 	if (!skb)
 		return -ENOMEM;
 
@@ -544,7 +544,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
 
-	ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, GFP_KERNEL);
+	ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, GFP_ATOMIC);
 	if (!ret)
 		ret = len;
 	else
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
should be atomic
GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
fail but certainly avoids deadlock

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
 drivers/infiniband/core/sa_query.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 8c014b3..cd1f911 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -526,7 +526,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
 	if (len <= 0)
 		return -EMSGSIZE;
 
-	skb = nlmsg_new(len, GFP_KERNEL);
+	skb = nlmsg_new(len, GFP_ATOMIC);
 	if (!skb)
 		return -ENOMEM;
 
@@ -544,7 +544,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
 	/* Repair the nlmsg header length */
 	nlmsg_end(skb, nlh);
 
-	ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, GFP_KERNEL);
+	ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, GFP_ATOMIC);
 	if (!ret)
 		ret = len;
 	else
-- 
1.9.1

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[ira.weiny]

On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> should be atomic
> GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> fail but certainly avoids deadlock

Great catch.  Thanks!

However, gfp_t is passed to send_mad and we should pass that down and use it.

Compile tested only, suggestion below,
Ira


14:09:12 > git di
diff --git a/drivers/infiniband/core/sa_query.c
b/drivers/infiniband/core/sa_query.c
index 8c014b33d8e0..54d454042b28 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -512,7 +512,7 @@ static int ib_nl_get_path_rec_attrs_len(ib_sa_comp_mask comp_mask)
        return len;
 }
 
-static int ib_nl_send_msg(struct ib_sa_query *query)
+static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 {
        struct sk_buff *skb = NULL;
        struct nlmsghdr *nlh;
@@ -526,7 +526,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
        if (len <= 0)
                return -EMSGSIZE;
 
-       skb = nlmsg_new(len, GFP_KERNEL);
+       skb = nlmsg_new(len, gfp_mask);
        if (!skb)
                return -ENOMEM;
 
@@ -544,7 +544,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
        /* Repair the nlmsg header length */
        nlmsg_end(skb, nlh);
 
-       ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, GFP_KERNEL);
+       ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, gfp_mask);
        if (!ret)
                ret = len;
        else
@@ -553,7 +553,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
        return ret;
 }
 
-static int ib_nl_make_request(struct ib_sa_query *query)
+static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
        unsigned long flags;
        unsigned long delay;
@@ -563,7 +563,7 @@ static int ib_nl_make_request(struct ib_sa_query *query)
        query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
        spin_lock_irqsave(&ib_nl_request_lock, flags);
-       ret = ib_nl_send_msg(query);
+       ret = ib_nl_send_msg(query, gfp_mask);
        if (ret <= 0) {
                ret = -EIO;
                goto request_out;
@@ -1105,7 +1105,7 @@ static int send_mad(struct ib_sa_query *query, int timeout_ms, gfp_t gfp_mask)
 
        if (query->flags & IB_SA_ENABLE_LOCAL_SERVICE) {
                if (!ibnl_chk_listeners(RDMA_NL_GROUP_LS)) {
-                       if (!ib_nl_make_request(query))
+                       if (!ib_nl_make_request(query, gfp_mask))
                                return id;
                }
                ib_sa_disable_local_svc(query);

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[ira.weiny]

On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> should be atomic
> GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> fail but certainly avoids deadlock

Great catch.  Thanks!

However, gfp_t is passed to send_mad and we should pass that down and use it.

Compile tested only, suggestion below,
Ira


14:09:12 > git di
diff --git a/drivers/infiniband/core/sa_query.c
b/drivers/infiniband/core/sa_query.c
index 8c014b33d8e0..54d454042b28 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -512,7 +512,7 @@ static int ib_nl_get_path_rec_attrs_len(ib_sa_comp_mask comp_mask)
        return len;
 }
 
-static int ib_nl_send_msg(struct ib_sa_query *query)
+static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 {
        struct sk_buff *skb = NULL;
        struct nlmsghdr *nlh;
@@ -526,7 +526,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
        if (len <= 0)
                return -EMSGSIZE;
 
-       skb = nlmsg_new(len, GFP_KERNEL);
+       skb = nlmsg_new(len, gfp_mask);
        if (!skb)
                return -ENOMEM;
 
@@ -544,7 +544,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
        /* Repair the nlmsg header length */
        nlmsg_end(skb, nlh);
 
-       ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, GFP_KERNEL);
+       ret = ibnl_multicast(skb, nlh, RDMA_NL_GROUP_LS, gfp_mask);
        if (!ret)
                ret = len;
        else
@@ -553,7 +553,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query)
        return ret;
 }
 
-static int ib_nl_make_request(struct ib_sa_query *query)
+static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 {
        unsigned long flags;
        unsigned long delay;
@@ -563,7 +563,7 @@ static int ib_nl_make_request(struct ib_sa_query *query)
        query->seq = (u32)atomic_inc_return(&ib_nl_sa_request_seq);
 
        spin_lock_irqsave(&ib_nl_request_lock, flags);
-       ret = ib_nl_send_msg(query);
+       ret = ib_nl_send_msg(query, gfp_mask);
        if (ret <= 0) {
                ret = -EIO;
                goto request_out;
@@ -1105,7 +1105,7 @@ static int send_mad(struct ib_sa_query *query, int timeout_ms, gfp_t gfp_mask)
 
        if (query->flags & IB_SA_ENABLE_LOCAL_SERVICE) {
                if (!ibnl_chk_listeners(RDMA_NL_GROUP_LS)) {
-                       if (!ib_nl_make_request(query))
+                       if (!ib_nl_make_request(query, gfp_mask))
                                return id;
                }
                ib_sa_disable_local_svc(query);

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Jason Gunthorpe]

On Tue, Oct 27, 2015 at 02:12:36PM -0400, ira.weiny wrote:
> On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> > replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> > should be atomic
> > GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> > fail but certainly avoids deadlock
> 
> Great catch.  Thanks!
> 
> However, gfp_t is passed to send_mad and we should pass that down and use it.

>         spin_lock_irqsave(&ib_nl_request_lock, flags);
> -       ret = ib_nl_send_msg(query);
> +       ret = ib_nl_send_msg(query, gfp_mask);

A spin lock is guarenteed held around ib_nl_send_msg, so it's
allocations have to be atomic, can't use gfp_mask here..

I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock
held though.. Would be nice to see that go away.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Jason Gunthorpe]

On Tue, Oct 27, 2015 at 02:12:36PM -0400, ira.weiny wrote:
> On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> > replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> > should be atomic
> > GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> > fail but certainly avoids deadlock
> 
> Great catch.  Thanks!
> 
> However, gfp_t is passed to send_mad and we should pass that down and use it.

>         spin_lock_irqsave(&ib_nl_request_lock, flags);
> -       ret = ib_nl_send_msg(query);
> +       ret = ib_nl_send_msg(query, gfp_mask);

A spin lock is guarenteed held around ib_nl_send_msg, so it's
allocations have to be atomic, can't use gfp_mask here..

I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock
held though.. Would be nice to see that go away.

Jason

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[ira.weiny]

On Tue, Oct 27, 2015 at 12:16:52PM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 27, 2015 at 02:12:36PM -0400, ira.weiny wrote:
> > On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> > > replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> > > should be atomic
> > > GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> > > fail but certainly avoids deadlock
> > 
> > Great catch.  Thanks!
> > 
> > However, gfp_t is passed to send_mad and we should pass that down and use it.
> 
> >         spin_lock_irqsave(&ib_nl_request_lock, flags);
> > -       ret = ib_nl_send_msg(query);
> > +       ret = ib_nl_send_msg(query, gfp_mask);
> 
> A spin lock is guarenteed held around ib_nl_send_msg, so it's
> allocations have to be atomic, can't use gfp_mask here..
> 
> I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock
> held though.. Would be nice to see that go away.

Ah, yea my bad.

Ira

> 
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[ira.weiny]

On Tue, Oct 27, 2015 at 12:16:52PM -0600, Jason Gunthorpe wrote:
> On Tue, Oct 27, 2015 at 02:12:36PM -0400, ira.weiny wrote:
> > On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> > > replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> > > should be atomic
> > > GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> > > fail but certainly avoids deadlock
> > 
> > Great catch.  Thanks!
> > 
> > However, gfp_t is passed to send_mad and we should pass that down and use it.
> 
> >         spin_lock_irqsave(&ib_nl_request_lock, flags);
> > -       ret = ib_nl_send_msg(query);
> > +       ret = ib_nl_send_msg(query, gfp_mask);
> 
> A spin lock is guarenteed held around ib_nl_send_msg, so it's
> allocations have to be atomic, can't use gfp_mask here..
> 
> I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock
> held though.. Would be nice to see that go away.

Ah, yea my bad.

Ira

> 
> Jason

RE: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Tuesday, October 27, 2015 2:17 PM
> To: Weiny, Ira
> Cc: Saurabh Sengar; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; Hefty, Sean;
> hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; yun.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org; Wan, Kaike; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC
> 
> On Tue, Oct 27, 2015 at 02:12:36PM -0400, ira.weiny wrote:
> > On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> > > replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> > > should be atomic GFP_KERNEL may sleep and can cause deadlock, where
> > > as GFP_ATOMIC may fail but certainly avoids deadlock
> >
> > Great catch.  Thanks!
> >
> > However, gfp_t is passed to send_mad and we should pass that down and
> use it.
> 
> >         spin_lock_irqsave(&ib_nl_request_lock, flags);
> > -       ret = ib_nl_send_msg(query);
> > +       ret = ib_nl_send_msg(query, gfp_mask);
> 
> A spin lock is guarenteed held around ib_nl_send_msg, so it's allocations
> have to be atomic, can't use gfp_mask here..
> 
> I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock held
> though.. Would be nice to see that go away.

We have to hold the lock to protect against a race condition that a quick response will try to free the request from the ib_nl_request_list before we even put it on the list.

> 
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com]
> Sent: Tuesday, October 27, 2015 2:17 PM
> To: Weiny, Ira
> Cc: Saurabh Sengar; dledford@redhat.com; Hefty, Sean;
> hal.rosenstock@gmail.com; yun.wang@profitbricks.com; Wan, Kaike; linux-
> rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC
> 
> On Tue, Oct 27, 2015 at 02:12:36PM -0400, ira.weiny wrote:
> > On Tue, Oct 27, 2015 at 09:17:40PM +0530, Saurabh Sengar wrote:
> > > replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> > > should be atomic GFP_KERNEL may sleep and can cause deadlock, where
> > > as GFP_ATOMIC may fail but certainly avoids deadlock
> >
> > Great catch.  Thanks!
> >
> > However, gfp_t is passed to send_mad and we should pass that down and
> use it.
> 
> >         spin_lock_irqsave(&ib_nl_request_lock, flags);
> > -       ret = ib_nl_send_msg(query);
> > +       ret = ib_nl_send_msg(query, gfp_mask);
> 
> A spin lock is guarenteed held around ib_nl_send_msg, so it's allocations
> have to be atomic, can't use gfp_mask here..
> 
> I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock held
> though.. Would be nice to see that go away.

We have to hold the lock to protect against a race condition that a quick response will try to free the request from the ib_nl_request_list before we even put it on the list.

> 
> Jason

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Jason Gunthorpe]

On Tue, Oct 27, 2015 at 06:56:50PM +0000, Wan, Kaike wrote:
 
> > I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock held
> > though.. Would be nice to see that go away.
> 
> We have to hold the lock to protect against a race condition that a
> quick response will try to free the request from the
> ib_nl_request_list before we even put it on the list.

Put is on the list first? Use a kref? Doesn't look like a big deal to
clean this up.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Jason Gunthorpe]

On Tue, Oct 27, 2015 at 06:56:50PM +0000, Wan, Kaike wrote:
 
> > I do wonder if it is a good idea to call ib_nl_send_msg with a spinlock held
> > though.. Would be nice to see that go away.
> 
> We have to hold the lock to protect against a race condition that a
> quick response will try to free the request from the
> ib_nl_request_list before we even put it on the list.

Put is on the list first? Use a kref? Doesn't look like a big deal to
clean this up.

Jason

RE: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Weiny, Ira]

> 
> On Tue, Oct 27, 2015 at 06:56:50PM +0000, Wan, Kaike wrote:
> 
> > > I do wonder if it is a good idea to call ib_nl_send_msg with a
> > > spinlock held though.. Would be nice to see that go away.
> >
> > We have to hold the lock to protect against a race condition that a
> > quick response will try to free the request from the
> > ib_nl_request_list before we even put it on the list.
> 
> Put is on the list first? Use a kref? Doesn't look like a big deal to clean this up.
> 
> Jason

Not sure what "Put is on the list first?" means.  I think it is valid to build the request, 
if success, add to the list, then send it.  That would solve the problem you mention 
above.  Was that what you hand in mind, Jason?

I don't have time to work on this right now, not sure about Kaike.  Until we can remove 
the spinlock the current proposed patch should be applied in the interim.  Sorry for the
noise before.

Reviewed-By: Ira Weiny <ira.weiny@intel.com>

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

On Wed, 28 Oct 2015  at 04:30:10 +0000, Weiny, Ira wrote:
> Until we can remove the spinlock the current proposed patch should be applied 
> in the interim.  Sorry for the noise before.

> Reviewed-By: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Hi,

is this patch expected to be applied ?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

On Wed, 28 Oct 2015  at 04:30:10 +0000, Weiny, Ira wrote:
> Until we can remove the spinlock the current proposed patch should be applied 
> in the interim.  Sorry for the noise before.

> Reviewed-By: Ira Weiny <ira.weiny@intel.com>

Hi,

is this patch expected to be applied ?

RE: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Wan, Kaike]

I don't think so. 

The following patch has rendered this patch unnecessary:

https://patchwork.kernel.org/patch/7526811/

Kaike

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Saurabh Sengar
> Sent: Friday, November 13, 2015 2:42 AM
> To: Weiny, Ira; Wan, Kaike
> Cc: Hefty, Sean; hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC
> 
> On Wed, 28 Oct 2015  at 04:30:10 +0000, Weiny, Ira wrote:
> > Until we can remove the spinlock the current proposed patch should be
> > applied in the interim.  Sorry for the noise before.
> 
> > Reviewed-By: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Hi,
> 
> is this patch expected to be applied ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
> body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Wan, Kaike]

I don't think so. 

The following patch has rendered this patch unnecessary:

https://patchwork.kernel.org/patch/7526811/

Kaike

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Saurabh Sengar
> Sent: Friday, November 13, 2015 2:42 AM
> To: Weiny, Ira; Wan, Kaike
> Cc: Hefty, Sean; hal.rosenstock@gmail.com; dledford@redhat.com; linux-
> rdma@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC
> 
> On Wed, 28 Oct 2015  at 04:30:10 +0000, Weiny, Ira wrote:
> > Until we can remove the spinlock the current proposed patch should be
> > applied in the interim.  Sorry for the noise before.
> 
> > Reviewed-By: Ira Weiny <ira.weiny@intel.com>
> 
> Hi,
> 
> is this patch expected to be applied ?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

On Fri, 13 Nov 2015  at 10:47:52 +0000, Wan, Kaike wrote:
> I don't think so.
> The following patch has rendered this patch unnecessary:
> https://patchwork.kernel.org/patch/7526811/
> Kaike

OK, could you please use "Reported-by" tag of my name in your patch

Regards,
Saurabh
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/sa: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

On Fri, 13 Nov 2015  at 10:47:52 +0000, Wan, Kaike wrote:
> I don't think so.
> The following patch has rendered this patch unnecessary:
> https://patchwork.kernel.org/patch/7526811/
> Kaike

OK, could you please use "Reported-by" tag of my name in your patch

Regards,
Saurabh

[PATCH] efi: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
should be atomic
GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
fail but certainly avoids deadlock

Signed-off-by: Saurabh Sengar <saurabh.truth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/vars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb1..c07de85 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -334,7 +334,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 	 */
 	efivar_wq_enabled = false;
 
-	str8 = kzalloc(len8, GFP_KERNEL);
+	str8 = kzalloc(len8, GFP_ATOMIC);
 	if (!str8)
 		return;
 
-- 
1.9.1

[PATCH] efi: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
should be atomic
GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
fail but certainly avoids deadlock

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
 drivers/firmware/efi/vars.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb1..c07de85 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -334,7 +334,7 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 	 */
 	efivar_wq_enabled = false;
 
-	str8 = kzalloc(len8, GFP_KERNEL);
+	str8 = kzalloc(len8, GFP_ATOMIC);
 	if (!str8)
 		return;
 
-- 
1.9.1

[PATCH v2] efi: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
should be atomic
GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
fail but certainly avoids deadlock

Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
---
 drivers/firmware/efi/vars.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 70a0fb1..d4eeebf 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -322,10 +322,11 @@ static unsigned long var_name_strnsize(efi_char16_t *variable_name,
  * disable the sysfs workqueue since the firmware is buggy.
  */
 static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
-			     unsigned long len16)
+			     unsigned long len16, bool atomic)
 {
 	size_t i, len8 = len16 / sizeof(efi_char16_t);
 	char *str8;
+	int gfp_mask;
 
 	/*
 	 * Disable the workqueue since the algorithm it uses for
@@ -334,7 +335,12 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
 	 */
 	efivar_wq_enabled = false;
 
-	str8 = kzalloc(len8, GFP_KERNEL);
+	if (atomic)
+		gfp_mask = GFP_ATOMIC;
+	else
+		gfp_mask = GFP_KERNEL;
+
+	str8 = kzalloc(len8, gfp_mask);
 	if (!str8)
 		return;
 
@@ -408,7 +414,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
 			if (duplicates &&
 			    variable_is_present(variable_name, &vendor_guid, head)) {
 				dup_variable_bug(variable_name, &vendor_guid,
-						 variable_name_size);
+						 variable_name_size, atomic);
 				if (!atomic)
 					spin_lock_irq(&__efivars->lock);
 
-- 
1.9.1

Re: [PATCH v2] efi: replace GFP_KERNEL with GFP_ATOMIC

[Matt Fleming]

On Wed, 28 Oct, at 09:12:27AM, Saurabh Sengar wrote:
> replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> should be atomic
> GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> fail but certainly avoids deadlock
> 
> Signed-off-by: Saurabh Sengar <saurabh.truth-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/vars.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 70a0fb1..d4eeebf 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -322,10 +322,11 @@ static unsigned long var_name_strnsize(efi_char16_t *variable_name,
>   * disable the sysfs workqueue since the firmware is buggy.
>   */
>  static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
> -			     unsigned long len16)
> +			     unsigned long len16, bool atomic)
>  {
>  	size_t i, len8 = len16 / sizeof(efi_char16_t);
>  	char *str8;
> +	int gfp_mask;
>  
>  	/*
>  	 * Disable the workqueue since the algorithm it uses for
> @@ -334,7 +335,12 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
>  	 */
>  	efivar_wq_enabled = false;
>  
> -	str8 = kzalloc(len8, GFP_KERNEL);
> +	if (atomic)
> +		gfp_mask = GFP_ATOMIC;
> +	else
> +		gfp_mask = GFP_KERNEL;
> +
> +	str8 = kzalloc(len8, gfp_mask);
>  	if (!str8)
>  		return;
>  
> @@ -408,7 +414,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  			if (duplicates &&
>  			    variable_is_present(variable_name, &vendor_guid, head)) {
>  				dup_variable_bug(variable_name, &vendor_guid,
> -						 variable_name_size);
> +						 variable_name_size, atomic);
>  				if (!atomic)
>  					spin_lock_irq(&__efivars->lock);

It's slightly winding code, but if you look at the callers of
efivar_init() you'll see that none of them set both 'atomic' and
'duplicates', so dup_variable_bug() will never be called while holding
a spinlock.

Or am I missing something?

Re: [PATCH v2] efi: replace GFP_KERNEL with GFP_ATOMIC

[Matt Fleming]

On Wed, 28 Oct, at 09:12:27AM, Saurabh Sengar wrote:
> replace GFP_KERNEL with GFP_ATOMIC, as code while holding a spinlock
> should be atomic
> GFP_KERNEL may sleep and can cause deadlock, where as GFP_ATOMIC may
> fail but certainly avoids deadlock
> 
> Signed-off-by: Saurabh Sengar <saurabh.truth@gmail.com>
> ---
>  drivers/firmware/efi/vars.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 70a0fb1..d4eeebf 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -322,10 +322,11 @@ static unsigned long var_name_strnsize(efi_char16_t *variable_name,
>   * disable the sysfs workqueue since the firmware is buggy.
>   */
>  static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
> -			     unsigned long len16)
> +			     unsigned long len16, bool atomic)
>  {
>  	size_t i, len8 = len16 / sizeof(efi_char16_t);
>  	char *str8;
> +	int gfp_mask;
>  
>  	/*
>  	 * Disable the workqueue since the algorithm it uses for
> @@ -334,7 +335,12 @@ static void dup_variable_bug(efi_char16_t *str16, efi_guid_t *vendor_guid,
>  	 */
>  	efivar_wq_enabled = false;
>  
> -	str8 = kzalloc(len8, GFP_KERNEL);
> +	if (atomic)
> +		gfp_mask = GFP_ATOMIC;
> +	else
> +		gfp_mask = GFP_KERNEL;
> +
> +	str8 = kzalloc(len8, gfp_mask);
>  	if (!str8)
>  		return;
>  
> @@ -408,7 +414,7 @@ int efivar_init(int (*func)(efi_char16_t *, efi_guid_t, unsigned long, void *),
>  			if (duplicates &&
>  			    variable_is_present(variable_name, &vendor_guid, head)) {
>  				dup_variable_bug(variable_name, &vendor_guid,
> -						 variable_name_size);
> +						 variable_name_size, atomic);
>  				if (!atomic)
>  					spin_lock_irq(&__efivars->lock);

It's slightly winding code, but if you look at the callers of
efivar_init() you'll see that none of them set both 'atomic' and
'duplicates', so dup_variable_bug() will never be called while holding
a spinlock.

Or am I missing something?

Re: [PATCH] efi: replace GFP_KERNEL with GFP_ATOMIC

[Saurabh Sengar]

>It's slightly winding code, but if you look at the callers of
>efivar_init() you'll see that none of them set both 'atomic' and
>'duplicates', so dup_variable_bug() will never be called while holding
>a spinlock.

>Or am I missing something?

I was assuming that there could be a possibility when both 'atomic' and 'duplicates' are set.
If both can never be set, then this patch does not make any sense, you are correct.
Thank you for review