As the kemdup could return NULL, it should be better to check the return
value and return error if fails.
Moreover, the return value of prestera_acl_ruleset_keymask_set() should
be checked by cascade.
Fixes: 604ba230902d ("net: prestera: flower template support")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/net/ethernet/marvell/prestera/prestera_acl.c | 8 ++++++--
drivers/net/ethernet/marvell/prestera/prestera_acl.h | 4 ++--
drivers/net/ethernet/marvell/prestera/prestera_flower.c | 6 +++++-
3 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_acl.c b/drivers/net/ethernet/marvell/prestera/prestera_acl.c
index 3d4b85f2d541..f6b2933859d0 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_acl.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_acl.c
@@ -178,10 +178,14 @@ prestera_acl_ruleset_create(struct prestera_acl *acl,
return ERR_PTR(err);
}
-void prestera_acl_ruleset_keymask_set(struct prestera_acl_ruleset *ruleset,
- void *keymask)
+int prestera_acl_ruleset_keymask_set(struct prestera_acl_ruleset *ruleset,
+ void *keymask)
{
ruleset->keymask = kmemdup(keymask, ACL_KEYMASK_SIZE, GFP_KERNEL);
+ if (!ruleset->keymask)
+ return -ENOMEM;
+
+ return 0;
}
int prestera_acl_ruleset_offload(struct prestera_acl_ruleset *ruleset)
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_acl.h b/drivers/net/ethernet/marvell/prestera/prestera_acl.h
index 03fc5b9dc925..131bfbc87cd7 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_acl.h
+++ b/drivers/net/ethernet/marvell/prestera/prestera_acl.h
@@ -185,8 +185,8 @@ struct prestera_acl_ruleset *
prestera_acl_ruleset_lookup(struct prestera_acl *acl,
struct prestera_flow_block *block,
u32 chain_index);
-void prestera_acl_ruleset_keymask_set(struct prestera_acl_ruleset *ruleset,
- void *keymask);
+int prestera_acl_ruleset_keymask_set(struct prestera_acl_ruleset *ruleset,
+ void *keymask);
bool prestera_acl_ruleset_is_offload(struct prestera_acl_ruleset *ruleset);
int prestera_acl_ruleset_offload(struct prestera_acl_ruleset *ruleset);
void prestera_acl_ruleset_put(struct prestera_acl_ruleset *ruleset);
diff --git a/drivers/net/ethernet/marvell/prestera/prestera_flower.c b/drivers/net/ethernet/marvell/prestera/prestera_flower.c
index 19d3b55c578e..cf551a8379ac 100644
--- a/drivers/net/ethernet/marvell/prestera/prestera_flower.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_flower.c
@@ -452,7 +452,9 @@ int prestera_flower_tmplt_create(struct prestera_flow_block *block,
}
/* preserve keymask/template to this ruleset */
- prestera_acl_ruleset_keymask_set(ruleset, rule.re_key.match.mask);
+ err = prestera_acl_ruleset_keymask_set(ruleset, rule.re_key.match.mask);
+ if (err)
+ goto err_ruleset_keymask_set;
/* skip error, as it is not possible to reject template operation,
* so, keep the reference to the ruleset for rules to be added
@@ -468,6 +470,8 @@ int prestera_flower_tmplt_create(struct prestera_flow_block *block,
list_add_rcu(&template->list, &block->template_list);
return 0;
+err_ruleset_keymask_set:
+ prestera_acl_ruleset_put(ruleset);
err_ruleset_get:
kfree(template);
err_malloc:
--
2.25.1
On Wed, 28 Sep 2022 17:20:24 +0800 Jiasheng Jiang wrote:
> As the kemdup could return NULL, it should be better to check the return
> value and return error if fails.
> Moreover, the return value of prestera_acl_ruleset_keymask_set() should
> be checked by cascade.
>
> Fixes: 604ba230902d ("net: prestera: flower template support")
> Signed-off-by: Jiasheng Jiang <[email protected]>
You must CC the authors of patch you're fixing.
get_maintainer will do that for you I don't understand why people can't
simply run that script :/ You CC linux-kernel for no apparent reason
yet you don't CC the guy who wrote the original patch.
If you could please explain what is going on maybe we can improve the
tooling or something.
On Fri, 30 Sep 2022 00:15:27 +0800 Jakub Kicinski wrote:
> On Wed, 28 Sep 2022 17:20:24 +0800 Jiasheng Jiang wrote:
>> As the kemdup could return NULL, it should be better to check the return
>> value and return error if fails.
>> Moreover, the return value of prestera_acl_ruleset_keymask_set() should
>> be checked by cascade.
>>
>> Fixes: 604ba230902d ("net: prestera: flower template support")
>> Signed-off-by: Jiasheng Jiang <[email protected]>
>
> You must CC the authors of patch you're fixing.
> get_maintainer will do that for you I don't understand why people can't
> simply run that script :/ You CC linux-kernel for no apparent reason
> yet you don't CC the guy who wrote the original patch.
> If you could please explain what is going on maybe we can improve the
> tooling or something.
Actually, I used get_maintainer scripts and the results is as follow:
"./scripts/get_maintainer.pl -f drivers/net/ethernet/marvell/prestera/prestera_acl.c"
Taras Chornyi <[email protected]> (supporter:MARVELL PRESTERA ETHERNET SWITCH DRIVER)
"David S. Miller" <[email protected]> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <[email protected]> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <[email protected]> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <[email protected]> (maintainer:NETWORKING DRIVERS)
[email protected] (open list:NETWORKING DRIVERS)
[email protected] (open list)
Therefore, I submitted my patch to the above addresses.
And this time I checked the fixes commit, and found that it has two
authors:
Signed-off-by: Volodymyr Mytnyk <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Maybe there is a problem of the script that misses one.
Anyway, I have already submitted the same patch and added
"[email protected]" this time.
Thanks,
Jiang
On Fri, 30 Sep 2022 13:03:17 +0800 Jiasheng Jiang wrote:
> Actually, I used get_maintainer scripts and the results is as follow:
> "./scripts/get_maintainer.pl -f drivers/net/ethernet/marvell/prestera/prestera_acl.c"
> Taras Chornyi <[email protected]> (supporter:MARVELL PRESTERA ETHERNET SWITCH DRIVER)
> "David S. Miller" <[email protected]> (maintainer:NETWORKING DRIVERS)
> Eric Dumazet <[email protected]> (maintainer:NETWORKING DRIVERS)
> Jakub Kicinski <[email protected]> (maintainer:NETWORKING DRIVERS)
> Paolo Abeni <[email protected]> (maintainer:NETWORKING DRIVERS)
> [email protected] (open list:NETWORKING DRIVERS)
> [email protected] (open list)
>
> Therefore, I submitted my patch to the above addresses.
>
> And this time I checked the fixes commit, and found that it has two
> authors:
> Signed-off-by: Volodymyr Mytnyk <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
>
> Maybe there is a problem of the script that misses one.
> Anyway, I have already submitted the same patch and added
> "[email protected]" this time.
Ha! So you do indeed use it in a way I wasn't expecting :S
Thanks for the explanation.
Joe, would you be okay to add a "big fat warning" to get_maintainer
when people try to use the -f flag? Maybe we can also change the message
that's displayed when the script is run without arguments to not
mention -f?
We're getting quite a few fixes which don't CC author, I'm guessing
Jiasheng's approach may be a common one.
On Fri, 2022-09-30 at 07:29 -0700, Jakub Kicinski wrote:
> On Fri, 30 Sep 2022 13:03:17 +0800 Jiasheng Jiang wrote:
> > Actually, I used get_maintainer scripts and the results is as follow:
> > "./scripts/get_maintainer.pl -f drivers/net/ethernet/marvell/prestera/prestera_acl.c"
> > Taras Chornyi <[email protected]> (supporter:MARVELL PRESTERA ETHERNET SWITCH DRIVER)
> > "David S. Miller" <[email protected]> (maintainer:NETWORKING DRIVERS)
> > Eric Dumazet <[email protected]> (maintainer:NETWORKING DRIVERS)
> > Jakub Kicinski <[email protected]> (maintainer:NETWORKING DRIVERS)
> > Paolo Abeni <[email protected]> (maintainer:NETWORKING DRIVERS)
> > [email protected] (open list:NETWORKING DRIVERS)
> > [email protected] (open list)
> >
> > Therefore, I submitted my patch to the above addresses.
> >
> > And this time I checked the fixes commit, and found that it has two
> > authors:
> > Signed-off-by: Volodymyr Mytnyk <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
IMO: If Volodymyr wants to be a maintainer here, he should put
his email as an entry in the MAINTAINERS file for the subsystem.
> > Maybe there is a problem of the script that misses one.
I don't think so. Maybe you have more evidence...
> > Anyway, I have already submitted the same patch and added
> > "[email protected]" this time.
>
> Ha! So you do indeed use it in a way I wasn't expecting :S
> Thanks for the explanation.
>
> Joe, would you be okay to add a "big fat warning" to get_maintainer
> when people try to use the -f flag?
No, not really. -f isn't required when the file is in git anyway.
> Maybe we can also change the message
> that's displayed when the script is run without arguments to not
> mention -f?
I think that's a poor idea as frequently the script isn't used
on patches but simply to identify the maintainers of a particular
file or subsystem.
> We're getting quite a few fixes which don't CC author, I'm guessing
> Jiasheng's approach may be a common one.
There's no great way to identify "author" or "original submitter"
and frequently the "original submitter" isn't a maintainer anyway.
On Fri, 30 Sep 2022 08:20:47 -0700 Joe Perches wrote:
> IMO: If Volodymyr wants to be a maintainer here, he should put
> his email as an entry in the MAINTAINERS file for the subsystem.
It's about Fixes tags, unfortunately having everyone of note listed
in MAINTAINERS is pretty much impossible. Even tho we are trying.
> > > Maybe there is a problem of the script that misses one.
>
> I don't think so. Maybe you have more evidence...
I'll CC you when I tell people to CC authors of patches under Fixes
going forward, I don't have a list going back.
> > > Anyway, I have already submitted the same patch and added
> > > "[email protected]" this time.
> >
> > Ha! So you do indeed use it in a way I wasn't expecting :S
> > Thanks for the explanation.
> >
> > Joe, would you be okay to add a "big fat warning" to get_maintainer
> > when people try to use the -f flag?
>
> No, not really. -f isn't required when the file is in git anyway.
Ah. Yeah. I'd make it error out when run on a source file without -f :S
> > Maybe we can also change the message
> > that's displayed when the script is run without arguments to not
> > mention -f?
>
> I think that's a poor idea as frequently the script isn't used
> on patches but simply to identify the maintainers of a particular
> file or subsystem.
Identify the maintainers and report a bug, or something else? As a
maintainer I can tell you that I don't see bug reports as often as I
see trivial patches from noobs which miss CCs. And I personally don't
think I ever used get_maintainer on anything else than a patch.
> > We're getting quite a few fixes which don't CC author, I'm guessing
> > Jiasheng's approach may be a common one.
>
> There's no great way to identify "author" or "original submitter"
> and frequently the "original submitter" isn't a maintainer anyway.
Confusing sentence. We want for people who s-o-b'd the commit under
Fixes to be CCed.
On Fri, 2022-09-30 at 08:44 -0700, Jakub Kicinski wrote:
> On Fri, 30 Sep 2022 08:20:47 -0700 Joe Perches wrote:
> > IMO: If Volodymyr wants to be a maintainer here, he should put
> > his email as an entry in the MAINTAINERS file for the subsystem.
>
> It's about Fixes tags, unfortunately having everyone of note listed
> in MAINTAINERS is pretty much impossible. Even tho we are trying.
>
> > > > Maybe there is a problem of the script that misses one.
> >
> > I don't think so. Maybe you have more evidence...
>
> I'll CC you when I tell people to CC authors of patches under Fixes
> going forward, I don't have a list going back.
>
> > > > Anyway, I have already submitted the same patch and added
> > > > "[email protected]" this time.
> > >
> > > Ha! So you do indeed use it in a way I wasn't expecting :S
> > > Thanks for the explanation.
> > >
> > > Joe, would you be okay to add a "big fat warning" to get_maintainer
> > > when people try to use the -f flag?
> >
> > No, not really. -f isn't required when the file is in git anyway.
>
> Ah. Yeah. I'd make it error out when run on a source file without -f :S
>
> > > Maybe we can also change the message
> > > that's displayed when the script is run without arguments to not
> > > mention -f?
> >
> > I think that's a poor idea as frequently the script isn't used
> > on patches but simply to identify the maintainers of a particular
> > file or subsystem.
>
> Identify the maintainers and report a bug, or something else? As a
> maintainer I can tell you that I don't see bug reports as often as I
> see trivial patches from noobs which miss CCs. And I personally don't
> think I ever used get_maintainer on anything else than a patch.
>
> > > We're getting quite a few fixes which don't CC author, I'm guessing
> > > Jiasheng's approach may be a common one.
> >
> > There's no great way to identify "author" or "original submitter"
> > and frequently the "original submitter" isn't a maintainer anyway.
>
> Confusing sentence. We want for people who s-o-b'd the commit under
> Fixes to be CCed.
If a file or a file modified by a patch is listed in the MAINTAINERS,
git history isn't used unless --git is specified.
For a patch, maybe the author and other SOBs of a commit specified
by a "Fixes:" line SHA-1 in the commit message could be added automatically.
On Fri, 30 Sep 2022 09:43:54 -0700 Joe Perches wrote:
> > > There's no great way to identify "author" or "original submitter"
> > > and frequently the "original submitter" isn't a maintainer anyway.
> >
> > Confusing sentence. We want for people who s-o-b'd the commit under
> > Fixes to be CCed.
>
> If a file or a file modified by a patch is listed in the MAINTAINERS,
> git history isn't used unless --git is specified.
>
> For a patch, maybe the author and other SOBs of a commit specified
> by a "Fixes:" line SHA-1 in the commit message could be added automatically.
Yes, git history isn't used, but the Fixes tag are consulted already
AFAICT. We just need to steer people towards running the script on
the patch.
$ git format-patch net/main~..net/main -o /tmp/
/tmp/0001-eth-alx-take-rtnl_lock-on-resume.patch
$ grep Fixes /tmp/0001-eth-alx-take-rtnl_lock-on-resume.patch
Fixes: 4a5fe57e7751 ("alx: use fine-grained locking instead of RTNL")
$ git show 4a5fe57e7751 --pretty='%an <%ae>' --no-patch
Johannes Berg <[email protected]>
$ ./scripts/get_maintainer.pl /tmp/0001-eth-alx-take-rtnl_lock-on-resume.patch | grep blame
"David S. Miller" <[email protected]> (maintainer:NETWORKING DRIVERS,commit_signer:2/4=50%,blamed_fixes:1/1=100%)
Johannes Berg <[email protected]> (blamed_fixes:1/1=100%)
On Fri, 2022-09-30 at 09:58 -0700, Jakub Kicinski wrote:
> On Fri, 30 Sep 2022 09:43:54 -0700 Joe Perches wrote:
> > > > There's no great way to identify "author" or "original submitter"
> > > > and frequently the "original submitter" isn't a maintainer anyway.
> > >
> > > Confusing sentence. We want for people who s-o-b'd the commit under
> > > Fixes to be CCed.
> >
> > If a file or a file modified by a patch is listed in the MAINTAINERS,
> > git history isn't used unless --git is specified.
> >
> > For a patch, maybe the author and other SOBs of a commit specified
> > by a "Fixes:" line SHA-1 in the commit message could be added automatically.
>
> Yes, git history isn't used, but the Fixes tag are consulted already
> AFAICT. We just need to steer people towards running the script on
> the patch.
>
> $ git format-patch net/main~..net/main -o /tmp/
> /tmp/0001-eth-alx-take-rtnl_lock-on-resume.patch
>
> $ grep Fixes /tmp/0001-eth-alx-take-rtnl_lock-on-resume.patch
> Fixes: 4a5fe57e7751 ("alx: use fine-grained locking instead of RTNL")
>
> $ git show 4a5fe57e7751 --pretty='%an <%ae>' --no-patch
> Johannes Berg <[email protected]>
>
> $ ./scripts/get_maintainer.pl /tmp/0001-eth-alx-take-rtnl_lock-on-resume.patch | grep blame
> "David S. Miller" <[email protected]> (maintainer:NETWORKING DRIVERS,commit_signer:2/4=50%,blamed_fixes:1/1=100%)
> Johannes Berg <[email protected]> (blamed_fixes:1/1=100%)
Yeah, you kinda ruined my reveal. It already does that.
Of course I did that 3 years ago and I forgot about it.
cheers, Joe