* [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-03 11:25 [PATCH 0/4] backports: more generic ethernet patches Stefan Assmann
@ 2015-06-03 11:25 ` Stefan Assmann
2015-06-03 20:13 ` Hauke Mehrtens
2015-06-03 11:25 ` [PATCH 2/4] backports: replace igb ethtool_cmd_mdix " Stefan Assmann
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stefan Assmann @ 2015-06-03 11:25 UTC (permalink / raw)
To: backports; +Cc: mcgrof, hauke, sassmann
Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
.../network/0035-skb_no_fcs/igb_skb_no_fcs.patch | 14 --------------
.../network/0035-skb_no_fcs/skb_no_fcs.cocci | 7 +++++++
2 files changed, 7 insertions(+), 14 deletions(-)
delete mode 100644 patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
create mode 100644 patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
diff --git a/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch b/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
deleted file mode 100644
index f659bfd..0000000
--- a/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
+++ /dev/null
@@ -1,14 +0,0 @@
---- a/drivers/net/ethernet/intel/igb/igb_main.c
-+++ b/drivers/net/ethernet/intel/igb/igb_main.c
-@@ -4782,9 +4782,10 @@ static u32 igb_tx_cmd_type(struct sk_buf
- cmd_type |= IGB_SET_FLAG(tx_flags, IGB_TX_FLAGS_TSTAMP,
- (E1000_ADVTXD_MAC_TSTAMP));
-
-+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0)
- /* insert frame checksum */
- cmd_type ^= IGB_SET_FLAG(skb->no_fcs, 1, E1000_ADVTXD_DCMD_IFCS);
--
-+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0) */
- return cmd_type;
- }
-
diff --git a/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci b/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
new file mode 100644
index 0000000..703c227
--- /dev/null
+++ b/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
@@ -0,0 +1,7 @@
+@r1@
+expression E1,E2;
+struct sk_buff *skb;
+@@
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)
+ E1 ^= E2(..., skb->no_fcs, ...)
++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0) */
--
2.4.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-03 11:25 ` [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch Stefan Assmann
@ 2015-06-03 20:13 ` Hauke Mehrtens
2015-06-04 9:08 ` Stefan Assmann
0 siblings, 1 reply; 13+ messages in thread
From: Hauke Mehrtens @ 2015-06-03 20:13 UTC (permalink / raw)
To: Stefan Assmann, backports; +Cc: mcgrof
On 06/03/2015 01:25 PM, Stefan Assmann wrote:
> Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
> ---
> .../network/0035-skb_no_fcs/igb_skb_no_fcs.patch | 14 --------------
> .../network/0035-skb_no_fcs/skb_no_fcs.cocci | 7 +++++++
> 2 files changed, 7 insertions(+), 14 deletions(-)
> delete mode 100644 patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
> create mode 100644 patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
>
> diff --git a/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch b/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
> deleted file mode 100644
> index f659bfd..0000000
> --- a/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
> +++ /dev/null
> @@ -1,14 +0,0 @@
> ---- a/drivers/net/ethernet/intel/igb/igb_main.c
> -+++ b/drivers/net/ethernet/intel/igb/igb_main.c
> -@@ -4782,9 +4782,10 @@ static u32 igb_tx_cmd_type(struct sk_buf
> - cmd_type |= IGB_SET_FLAG(tx_flags, IGB_TX_FLAGS_TSTAMP,
> - (E1000_ADVTXD_MAC_TSTAMP));
> -
> -+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0)
> - /* insert frame checksum */
> - cmd_type ^= IGB_SET_FLAG(skb->no_fcs, 1, E1000_ADVTXD_DCMD_IFCS);
> --
> -+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0) */
> - return cmd_type;
> - }
> -
> diff --git a/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci b/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
> new file mode 100644
> index 0000000..703c227
> --- /dev/null
> +++ b/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
> @@ -0,0 +1,7 @@
> +@r1@
> +expression E1,E2;
> +struct sk_buff *skb;
> +@@
> ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)
> + E1 ^= E2(..., skb->no_fcs, ...)
> ++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0) */
>
Is it always save to just remove something which accesses skb->no_fcs in
all cases? I think sometimes some special handling for older kernel
version could be needed. This also looks very specific to the igb usage.
Hauke
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-03 20:13 ` Hauke Mehrtens
@ 2015-06-04 9:08 ` Stefan Assmann
2015-06-08 22:09 ` Luis R. Rodriguez
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Assmann @ 2015-06-04 9:08 UTC (permalink / raw)
To: Hauke Mehrtens, backports; +Cc: mcgrof
On 03.06.2015 22:13, Hauke Mehrtens wrote:
> On 06/03/2015 01:25 PM, Stefan Assmann wrote:
>> Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
>> ---
>> .../network/0035-skb_no_fcs/igb_skb_no_fcs.patch | 14 --------------
>> .../network/0035-skb_no_fcs/skb_no_fcs.cocci | 7 +++++++
>> 2 files changed, 7 insertions(+), 14 deletions(-)
>> delete mode 100644 patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
>> create mode 100644 patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
>>
>> diff --git a/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch b/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
>> deleted file mode 100644
>> index f659bfd..0000000
>> --- a/patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch
>> +++ /dev/null
>> @@ -1,14 +0,0 @@
>> ---- a/drivers/net/ethernet/intel/igb/igb_main.c
>> -+++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> -@@ -4782,9 +4782,10 @@ static u32 igb_tx_cmd_type(struct sk_buf
>> - cmd_type |= IGB_SET_FLAG(tx_flags, IGB_TX_FLAGS_TSTAMP,
>> - (E1000_ADVTXD_MAC_TSTAMP));
>> -
>> -+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0)
>> - /* insert frame checksum */
>> - cmd_type ^= IGB_SET_FLAG(skb->no_fcs, 1, E1000_ADVTXD_DCMD_IFCS);
>> --
>> -+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(3,4,0) */
>> - return cmd_type;
>> - }
>> -
>> diff --git a/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci b/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
>> new file mode 100644
>> index 0000000..703c227
>> --- /dev/null
>> +++ b/patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci
>> @@ -0,0 +1,7 @@
>> +@r1@
>> +expression E1,E2;
>> +struct sk_buff *skb;
>> +@@
>> ++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)
>> + E1 ^= E2(..., skb->no_fcs, ...)
>> ++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0) */
>>
>
> Is it always save to just remove something which accesses skb->no_fcs in
> all cases? I think sometimes some special handling for older kernel
> version could be needed. This also looks very specific to the igb usage.
>
> Hauke
>
In this case I'd say this is fine, no_fcs is used to send out frames with
bad CRC for testing. So just commenting out related code shouldn't cause
any harm. Yes, the cocci code is very specific and will likely need to be
extended for other drivers when we pull them in. But you have to start
somewhere.
We always have the option to revert if something turns out to be a bad
idea.
Stefan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-04 9:08 ` Stefan Assmann
@ 2015-06-08 22:09 ` Luis R. Rodriguez
2015-06-09 6:46 ` Stefan Assmann
0 siblings, 1 reply; 13+ messages in thread
From: Luis R. Rodriguez @ 2015-06-08 22:09 UTC (permalink / raw)
To: Stefan Assmann; +Cc: Hauke Mehrtens, backports@vger.kernel.org
On Thu, Jun 4, 2015 at 2:08 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
> On 03.06.2015 22:13, Hauke Mehrtens wrote:
>> Is it always save to just remove something which accesses skb->no_fcs in
>> all cases? I think sometimes some special handling for older kernel
>> version could be needed. This also looks very specific to the igb usage.
>
> In this case I'd say this is fine, no_fcs is used to send out frames with
> bad CRC for testing. So just commenting out related code shouldn't cause
> any harm. Yes, the cocci code is very specific and will likely need to be
> extended for other drivers when we pull them in. But you have to start
> somewhere.
>
> We always have the option to revert if something turns out to be a bad
> idea.
I'm fine with merging now but please note the discussion, Hauke was
curious about the generality over the Coccinelle patch replacement
over a patch series. In order to help maintainers make a proper
assessment over whether or not we can merge an SmPL patch to replace a
patch series it is extremely useful to annotate the SmPL patch with
header comments which track:
a) The respective upstream commit and kernel version which introduced
the collateral evolution for which you are generalizing
b) A good description which explains your understanding as to why this
should work and will not break run time
a) is easy, b) is hard but it is the least we can do and I think we
will remain sane if we put this as a litmus test for future SmPL patch
replacements. Its not easy but please see my own SmPL patches and
review the description. Its pretty lengthy but these discussions can
be avoided if we had someone do the full homework.
If we want to scale we need this. Are you folks OK with requiring a)
and b) on future SmPL patch replacements? Best effort at least. Keep
in mind reason for this is also I believe there are some further
generalizations we can reach if we follow these best practices which I
think will have further beneficial gains to us.
Luis
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-08 22:09 ` Luis R. Rodriguez
@ 2015-06-09 6:46 ` Stefan Assmann
2015-06-09 18:24 ` Luis R. Rodriguez
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Assmann @ 2015-06-09 6:46 UTC (permalink / raw)
To: Luis R. Rodriguez; +Cc: Hauke Mehrtens, backports@vger.kernel.org
On 09.06.2015 00:09, Luis R. Rodriguez wrote:
> On Thu, Jun 4, 2015 at 2:08 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
>> On 03.06.2015 22:13, Hauke Mehrtens wrote:
>>> Is it always save to just remove something which accesses skb->no_fcs in
>>> all cases? I think sometimes some special handling for older kernel
>>> version could be needed. This also looks very specific to the igb usage.
>>
>> In this case I'd say this is fine, no_fcs is used to send out frames with
>> bad CRC for testing. So just commenting out related code shouldn't cause
>> any harm. Yes, the cocci code is very specific and will likely need to be
>> extended for other drivers when we pull them in. But you have to start
>> somewhere.
>>
>> We always have the option to revert if something turns out to be a bad
>> idea.
>
> I'm fine with merging now but please note the discussion, Hauke was
> curious about the generality over the Coccinelle patch replacement
> over a patch series. In order to help maintainers make a proper
> assessment over whether or not we can merge an SmPL patch to replace a
> patch series it is extremely useful to annotate the SmPL patch with
> header comments which track:
>
> a) The respective upstream commit and kernel version which introduced
> the collateral evolution for which you are generalizing
> b) A good description which explains your understanding as to why this
> should work and will not break run time
>
> a) is easy, b) is hard but it is the least we can do and I think we
> will remain sane if we put this as a litmus test for future SmPL patch
> replacements. Its not easy but please see my own SmPL patches and
> review the description. Its pretty lengthy but these discussions can
> be avoided if we had someone do the full homework.
>
> If we want to scale we need this. Are you folks OK with requiring a)
> and b) on future SmPL patch replacements? Best effort at least. Keep
> in mind reason for this is also I believe there are some further
> generalizations we can reach if we follow these best practices which I
> think will have further beneficial gains to us.
Adding that info to the git commit log sounds like added value. I'll try
to provide it in my next patchset. If I get you right, this is somewhat
similar to what's in the INFO files, correct?
Stefan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-09 6:46 ` Stefan Assmann
@ 2015-06-09 18:24 ` Luis R. Rodriguez
2015-06-09 21:46 ` Hauke Mehrtens
0 siblings, 1 reply; 13+ messages in thread
From: Luis R. Rodriguez @ 2015-06-09 18:24 UTC (permalink / raw)
To: Stefan Assmann; +Cc: Hauke Mehrtens, backports@vger.kernel.org
On Mon, Jun 8, 2015 at 11:46 PM, Stefan Assmann <sassmann@kpanic.de> wrote:
> On 09.06.2015 00:09, Luis R. Rodriguez wrote:
>> On Thu, Jun 4, 2015 at 2:08 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
>>> On 03.06.2015 22:13, Hauke Mehrtens wrote:
>>>> Is it always save to just remove something which accesses skb->no_fcs in
>>>> all cases? I think sometimes some special handling for older kernel
>>>> version could be needed. This also looks very specific to the igb usage.
>>>
>>> In this case I'd say this is fine, no_fcs is used to send out frames with
>>> bad CRC for testing. So just commenting out related code shouldn't cause
>>> any harm. Yes, the cocci code is very specific and will likely need to be
>>> extended for other drivers when we pull them in. But you have to start
>>> somewhere.
>>>
>>> We always have the option to revert if something turns out to be a bad
>>> idea.
>>
>> I'm fine with merging now but please note the discussion, Hauke was
>> curious about the generality over the Coccinelle patch replacement
>> over a patch series. In order to help maintainers make a proper
>> assessment over whether or not we can merge an SmPL patch to replace a
>> patch series it is extremely useful to annotate the SmPL patch with
>> header comments which track:
>>
>> a) The respective upstream commit and kernel version which introduced
>> the collateral evolution for which you are generalizing
>> b) A good description which explains your understanding as to why this
>> should work and will not break run time
>>
>> a) is easy, b) is hard but it is the least we can do and I think we
>> will remain sane if we put this as a litmus test for future SmPL patch
>> replacements. Its not easy but please see my own SmPL patches and
>> review the description. Its pretty lengthy but these discussions can
>> be avoided if we had someone do the full homework.
>>
>> If we want to scale we need this. Are you folks OK with requiring a)
>> and b) on future SmPL patch replacements? Best effort at least. Keep
>> in mind reason for this is also I believe there are some further
>> generalizations we can reach if we follow these best practices which I
>> think will have further beneficial gains to us.
>
> Adding that info to the git commit log sounds like added value. I'll try
> to provide it in my next patchset. If I get you right, this is somewhat
> similar to what's in the INFO files, correct?
The commit log can use it, but indeed it'd be useful if we keep track
of it also in the INFO files as well. In practice if its one file (one
patch or one cocci file) at the top as a comment is used as well for
info... The INFO file thing came about from the fact that Johannes
long ago streamlined the practice of splitting up patch series into
separate files, one for each affected module / component. This was
done to enable being able to skip patch files which you do not carry
on your copy target. So say you only want to provide backports tarball
that carried in only one device driver, one patch series might affect
all drivers but with this strategy the patching sequence would
continue after only applying the one patch for that series that is
applicable to that driver. Its redudnant to copy the same commit log /
verbose message to all patches so Johannes figured to extract that and
put it on a single INFO file.
Come to think of it, even if we have one file for a series whether its
regular patches or cocci patches perhaps its best we embrace the
practice of using the INFO strategy. This would help for example in
enabling evaluation of number of lines of patch series and cocci files
by just using 'wc -l', instead of having to first extract the info
somehow. I'll go ahead and make these changes, unless I hear
otherwise. Moving forward if we can stick to this that'd be great.
Luis
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-09 18:24 ` Luis R. Rodriguez
@ 2015-06-09 21:46 ` Hauke Mehrtens
2015-06-09 22:12 ` Luis R. Rodriguez
0 siblings, 1 reply; 13+ messages in thread
From: Hauke Mehrtens @ 2015-06-09 21:46 UTC (permalink / raw)
To: Luis R. Rodriguez, Stefan Assmann; +Cc: backports@vger.kernel.org
On 06/09/2015 08:24 PM, Luis R. Rodriguez wrote:
> On Mon, Jun 8, 2015 at 11:46 PM, Stefan Assmann <sassmann@kpanic.de> wrote:
>> On 09.06.2015 00:09, Luis R. Rodriguez wrote:
>>> On Thu, Jun 4, 2015 at 2:08 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
>>>> On 03.06.2015 22:13, Hauke Mehrtens wrote:
>>>>> Is it always save to just remove something which accesses skb->no_fcs in
>>>>> all cases? I think sometimes some special handling for older kernel
>>>>> version could be needed. This also looks very specific to the igb usage.
>>>>
>>>> In this case I'd say this is fine, no_fcs is used to send out frames with
>>>> bad CRC for testing. So just commenting out related code shouldn't cause
>>>> any harm. Yes, the cocci code is very specific and will likely need to be
>>>> extended for other drivers when we pull them in. But you have to start
>>>> somewhere.
>>>>
>>>> We always have the option to revert if something turns out to be a bad
>>>> idea.
>>>
>>> I'm fine with merging now but please note the discussion, Hauke was
>>> curious about the generality over the Coccinelle patch replacement
>>> over a patch series. In order to help maintainers make a proper
>>> assessment over whether or not we can merge an SmPL patch to replace a
>>> patch series it is extremely useful to annotate the SmPL patch with
>>> header comments which track:
>>>
>>> a) The respective upstream commit and kernel version which introduced
>>> the collateral evolution for which you are generalizing
>>> b) A good description which explains your understanding as to why this
>>> should work and will not break run time
>>>
>>> a) is easy, b) is hard but it is the least we can do and I think we
>>> will remain sane if we put this as a litmus test for future SmPL patch
>>> replacements. Its not easy but please see my own SmPL patches and
>>> review the description. Its pretty lengthy but these discussions can
>>> be avoided if we had someone do the full homework.
>>>
>>> If we want to scale we need this. Are you folks OK with requiring a)
>>> and b) on future SmPL patch replacements? Best effort at least. Keep
>>> in mind reason for this is also I believe there are some further
>>> generalizations we can reach if we follow these best practices which I
>>> think will have further beneficial gains to us.
>>
>> Adding that info to the git commit log sounds like added value. I'll try
>> to provide it in my next patchset. If I get you right, this is somewhat
>> similar to what's in the INFO files, correct?
>
> The commit log can use it, but indeed it'd be useful if we keep track
> of it also in the INFO files as well. In practice if its one file (one
> patch or one cocci file) at the top as a comment is used as well for
> info... The INFO file thing came about from the fact that Johannes
> long ago streamlined the practice of splitting up patch series into
> separate files, one for each affected module / component. This was
> done to enable being able to skip patch files which you do not carry
> on your copy target. So say you only want to provide backports tarball
> that carried in only one device driver, one patch series might affect
> all drivers but with this strategy the patching sequence would
> continue after only applying the one patch for that series that is
> applicable to that driver. Its redudnant to copy the same commit log /
> verbose message to all patches so Johannes figured to extract that and
> put it on a single INFO file.
>
> Come to think of it, even if we have one file for a series whether its
> regular patches or cocci patches perhaps its best we embrace the
> practice of using the INFO strategy. This would help for example in
> enabling evaluation of number of lines of patch series and cocci files
> by just using 'wc -l', instead of having to first extract the info
> somehow. I'll go ahead and make these changes, unless I hear
> otherwise. Moving forward if we can stick to this that'd be great.
I think it would be easier to put the description into the semantic
patch. When I open a semantic patch I have the description right there
and do not have to open an other file to get the description, this
applies for reading and also for updating the description. I think we
should put generating statistics to a very low priority.
The problem with semantic patches is that it is not obvious to which
file they are applied like it is with normal patches, so I think a
better description is needed.
I wanted to raise the problem that semantic patches are probably fine
and tested with one driver, but it would also apply to other drivers and
could also apply to them later, when some driver introduces this feature
later on, so you do not know where your semantic patch will be applied
when sending it. This is not a problem with this specific semantic patch
as you said removing the feature should not cause any harm in general.
Hauke
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch
2015-06-09 21:46 ` Hauke Mehrtens
@ 2015-06-09 22:12 ` Luis R. Rodriguez
0 siblings, 0 replies; 13+ messages in thread
From: Luis R. Rodriguez @ 2015-06-09 22:12 UTC (permalink / raw)
To: Hauke Mehrtens; +Cc: Stefan Assmann, backports@vger.kernel.org
On Tue, Jun 9, 2015 at 2:46 PM, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 06/09/2015 08:24 PM, Luis R. Rodriguez wrote:
>> On Mon, Jun 8, 2015 at 11:46 PM, Stefan Assmann <sassmann@kpanic.de> wrote:
>>> On 09.06.2015 00:09, Luis R. Rodriguez wrote:
>>>> On Thu, Jun 4, 2015 at 2:08 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
>>>>> On 03.06.2015 22:13, Hauke Mehrtens wrote:
>>>>>> Is it always save to just remove something which accesses skb->no_fcs in
>>>>>> all cases? I think sometimes some special handling for older kernel
>>>>>> version could be needed. This also looks very specific to the igb usage.
>>>>>
>>>>> In this case I'd say this is fine, no_fcs is used to send out frames with
>>>>> bad CRC for testing. So just commenting out related code shouldn't cause
>>>>> any harm. Yes, the cocci code is very specific and will likely need to be
>>>>> extended for other drivers when we pull them in. But you have to start
>>>>> somewhere.
>>>>>
>>>>> We always have the option to revert if something turns out to be a bad
>>>>> idea.
>>>>
>>>> I'm fine with merging now but please note the discussion, Hauke was
>>>> curious about the generality over the Coccinelle patch replacement
>>>> over a patch series. In order to help maintainers make a proper
>>>> assessment over whether or not we can merge an SmPL patch to replace a
>>>> patch series it is extremely useful to annotate the SmPL patch with
>>>> header comments which track:
>>>>
>>>> a) The respective upstream commit and kernel version which introduced
>>>> the collateral evolution for which you are generalizing
>>>> b) A good description which explains your understanding as to why this
>>>> should work and will not break run time
>>>>
>>>> a) is easy, b) is hard but it is the least we can do and I think we
>>>> will remain sane if we put this as a litmus test for future SmPL patch
>>>> replacements. Its not easy but please see my own SmPL patches and
>>>> review the description. Its pretty lengthy but these discussions can
>>>> be avoided if we had someone do the full homework.
>>>>
>>>> If we want to scale we need this. Are you folks OK with requiring a)
>>>> and b) on future SmPL patch replacements? Best effort at least. Keep
>>>> in mind reason for this is also I believe there are some further
>>>> generalizations we can reach if we follow these best practices which I
>>>> think will have further beneficial gains to us.
>>>
>>> Adding that info to the git commit log sounds like added value. I'll try
>>> to provide it in my next patchset. If I get you right, this is somewhat
>>> similar to what's in the INFO files, correct?
>>
>> The commit log can use it, but indeed it'd be useful if we keep track
>> of it also in the INFO files as well. In practice if its one file (one
>> patch or one cocci file) at the top as a comment is used as well for
>> info... The INFO file thing came about from the fact that Johannes
>> long ago streamlined the practice of splitting up patch series into
>> separate files, one for each affected module / component. This was
>> done to enable being able to skip patch files which you do not carry
>> on your copy target. So say you only want to provide backports tarball
>> that carried in only one device driver, one patch series might affect
>> all drivers but with this strategy the patching sequence would
>> continue after only applying the one patch for that series that is
>> applicable to that driver. Its redudnant to copy the same commit log /
>> verbose message to all patches so Johannes figured to extract that and
>> put it on a single INFO file.
>>
>> Come to think of it, even if we have one file for a series whether its
>> regular patches or cocci patches perhaps its best we embrace the
>> practice of using the INFO strategy. This would help for example in
>> enabling evaluation of number of lines of patch series and cocci files
>> by just using 'wc -l', instead of having to first extract the info
>> somehow. I'll go ahead and make these changes, unless I hear
>> otherwise. Moving forward if we can stick to this that'd be great.
>
> I think it would be easier to put the description into the semantic
> patch. When I open a semantic patch I have the description right there
> and do not have to open an other file to get the description, this
> applies for reading and also for updating the description.
OK works with me. So for patch series use INFO, for semantic patches
with just one file just put the description stuff and justification at
the top as a comment.
A good example:
https://git.kernel.org/cgit/linux/kernel/git/backports/backports.git/tree/patches/collateral-evolutions/network/0031-sk_data_ready.cocci
> I think we
> should put generating statistics to a very low priority.
Sure. My idea with this was to eventually just add support into
gentree.py with --gitdebug is used some minor statistics in the
temporary git tree created when applying each Coccinelle patch. For
instance over time Ethernet SmPL patches will increase their coverage
of what files they apply to as we add more Ethernet drivers, it'd be
great to find out exactly how many lines of code in terms of impact an
SmPL patch might have over time. With this being added into the commit
log when --gitdebug is used folks who want to generate stats could use
that option when doing a review of stats.
> The problem with semantic patches is that it is not obvious to which
> file they are applied like it is with normal patches, so I think a
> better description is needed.
Indeed - although its not obvious to what files an SmPL patch applies
the grammar used should narrow the scope of applicability, its why I
tend to use a needle-in-a-hay-stack approach for my first rules: first
I set a rule to narrow exactly what type of device I want to apply the
grammar patch to, and then I use this to zero in on the are I wish to
change.
> I wanted to raise the problem that semantic patches are probably fine
> and tested with one driver, but it would also apply to other drivers and
> could also apply to them later, when some driver introduces this feature
> later on, so you do not know where your semantic patch will be applied
> when sending it.
Indeed, so although at first glance its seen as a problem, in practice
a properly written SmPL backport -- one where the backporter took the
time to review the broad grammatical application of the patch over an
entire tree and its implications, as well as consider a possible
needle-in-the-haystack approach -- should yield *proper* new automatic
extensions of the backport onto other drivers. This is a benefit, not
a drawback of using Coccinelle, if done right (TM).
> This is not a problem with this specific semantic patch
> as you said removing the feature should not cause any harm in general.
Its an example of where upon review of the broad application is
determined and vetted to be logically OK. That's certainly we want
folks doing over each and every SmPL patch added to backports, we want
that documentation as well.
Luis
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] backports: replace igb ethtool_cmd_mdix patch with semantic patch
2015-06-03 11:25 [PATCH 0/4] backports: more generic ethernet patches Stefan Assmann
2015-06-03 11:25 ` [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch Stefan Assmann
@ 2015-06-03 11:25 ` Stefan Assmann
2015-06-03 11:25 ` [PATCH 3/4] backports: replace igb xmit_more " Stefan Assmann
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Stefan Assmann @ 2015-06-03 11:25 UTC (permalink / raw)
To: backports; +Cc: mcgrof, hauke, sassmann
Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
.../0040-ethtool_cmd_mdix/ethtool_cmd_mdix.cocci | 14 +++++++
.../igb_ethtool_cmd_mdix.patch | 47 ----------------------
2 files changed, 14 insertions(+), 47 deletions(-)
create mode 100644 patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/ethtool_cmd_mdix.cocci
delete mode 100644 patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/igb_ethtool_cmd_mdix.patch
diff --git a/patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/ethtool_cmd_mdix.cocci b/patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/ethtool_cmd_mdix.cocci
new file mode 100644
index 0000000..b32973b
--- /dev/null
+++ b/patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/ethtool_cmd_mdix.cocci
@@ -0,0 +1,14 @@
+@r1@
+expression E1;
+struct ethtool_cmd *ecmd;
+@@
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0)
+ ecmd->eth_tp_mdix_ctrl = E1;
++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0) */
+
+@r2@
+struct ethtool_cmd *ecmd;
+@@
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0)
+ if (ecmd->eth_tp_mdix_ctrl) {...}
++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0) */
diff --git a/patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/igb_ethtool_cmd_mdix.patch b/patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/igb_ethtool_cmd_mdix.patch
deleted file mode 100644
index f48aa3f..0000000
--- a/patches/collateral-evolutions/network/0040-ethtool_cmd_mdix/igb_ethtool_cmd_mdix.patch
+++ /dev/null
@@ -1,47 +0,0 @@
---- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
-+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
-@@ -247,10 +247,12 @@ static int igb_get_settings(struct net_d
- else
- ecmd->eth_tp_mdix = ETH_TP_MDI_INVALID;
-
-+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0)
- if (hw->phy.mdix == AUTO_ALL_MODES)
- ecmd->eth_tp_mdix_ctrl = ETH_TP_MDI_AUTO;
- else
- ecmd->eth_tp_mdix_ctrl = hw->phy.mdix;
-+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0) */
-
- return 0;
- }
-@@ -269,6 +271,7 @@ static int igb_set_settings(struct net_d
- return -EINVAL;
- }
-
-+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0)
- /* MDI setting is only allowed when autoneg enabled because
- * some hardware doesn't allow MDI setting when speed or
- * duplex is forced.
-@@ -283,6 +286,7 @@ static int igb_set_settings(struct net_d
- return -EINVAL;
- }
- }
-+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0) */
-
- while (test_and_set_bit(__IGB_RESETTING, &adapter->state))
- usleep_range(1000, 2000);
-@@ -326,6 +330,7 @@ static int igb_set_settings(struct net_d
- }
- }
-
-+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0)
- /* MDI-X => 2; MDI => 1; Auto => 3 */
- if (ecmd->eth_tp_mdix_ctrl) {
- /* fix up the value for auto (3 => 0) as zero is mapped
-@@ -336,6 +341,7 @@ static int igb_set_settings(struct net_d
- else
- hw->phy.mdix = ecmd->eth_tp_mdix_ctrl;
- }
-+#endif /* LINUX_VERSION_CODE >= KERNEL_VERSION(3,7,0) */
-
- /* reset the link */
- if (netif_running(adapter->netdev)) {
--
2.4.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 3/4] backports: replace igb xmit_more patch with semantic patch
2015-06-03 11:25 [PATCH 0/4] backports: more generic ethernet patches Stefan Assmann
2015-06-03 11:25 ` [PATCH 1/4] backports: replace igb skb->no_fcs patch with semantic patch Stefan Assmann
2015-06-03 11:25 ` [PATCH 2/4] backports: replace igb ethtool_cmd_mdix " Stefan Assmann
@ 2015-06-03 11:25 ` Stefan Assmann
2015-06-03 11:25 ` [PATCH 4/4] backports: replace igb pfmemalloc " Stefan Assmann
2015-06-09 22:15 ` [PATCH 0/4] backports: more generic ethernet patches Luis R. Rodriguez
4 siblings, 0 replies; 13+ messages in thread
From: Stefan Assmann @ 2015-06-03 11:25 UTC (permalink / raw)
To: backports; +Cc: mcgrof, hauke, sassmann
Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
.../network/0048-no_ndo_xmit_flush/igb.patch | 14 --------------
.../collateral-evolutions/network/0059-skb_xmit_more/INFO | 10 ++++++++++
.../network/0059-skb_xmit_more/skb_no_xmit_more.cocci | 9 +++++++++
3 files changed, 19 insertions(+), 14 deletions(-)
delete mode 100644 patches/collateral-evolutions/network/0048-no_ndo_xmit_flush/igb.patch
create mode 100644 patches/collateral-evolutions/network/0059-skb_xmit_more/INFO
create mode 100644 patches/collateral-evolutions/network/0059-skb_xmit_more/skb_no_xmit_more.cocci
diff --git a/patches/collateral-evolutions/network/0048-no_ndo_xmit_flush/igb.patch b/patches/collateral-evolutions/network/0048-no_ndo_xmit_flush/igb.patch
deleted file mode 100644
index 54d52a5..0000000
--- a/patches/collateral-evolutions/network/0048-no_ndo_xmit_flush/igb.patch
+++ /dev/null
@@ -1,14 +0,0 @@
---- a/drivers/net/ethernet/intel/igb/igb_main.c
-+++ b/drivers/net/ethernet/intel/igb/igb_main.c
-@@ -4952,7 +4952,10 @@ static void igb_tx_map(struct igb_ring *
- /* Make sure there is space in the ring for the next send. */
- igb_maybe_stop_tx(tx_ring, DESC_NEEDED);
-
-- if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more) {
-+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)
-+ if (netif_xmit_stopped(txring_txq(tx_ring)) || !skb->xmit_more)
-+#endif
-+ {
- writel(i, tx_ring->tail);
-
- /* we need this if more than one processor can write to our tail
diff --git a/patches/collateral-evolutions/network/0059-skb_xmit_more/INFO b/patches/collateral-evolutions/network/0059-skb_xmit_more/INFO
new file mode 100644
index 0000000..b429637
--- /dev/null
+++ b/patches/collateral-evolutions/network/0059-skb_xmit_more/INFO
@@ -0,0 +1,10 @@
+In kernel 3.18 __u8 xmit_more is added to struct sk_buff.
+
+commit 0b725a2ca61bedc33a2a63d0451d528b268cf975
+Author: David S. Miller <davem@davemloft.net>
+Date: Mon Aug 25 15:51:53 2014 -0700
+
+ net: Remove ndo_xmit_flush netdev operation, use signalling instead.
+
+git describe --contains 0b725a2ca61bedc33a2a63d0451d528b268cf975
+v3.18-rc1~52^2~228
diff --git a/patches/collateral-evolutions/network/0059-skb_xmit_more/skb_no_xmit_more.cocci b/patches/collateral-evolutions/network/0059-skb_xmit_more/skb_no_xmit_more.cocci
new file mode 100644
index 0000000..ab207bd
--- /dev/null
+++ b/patches/collateral-evolutions/network/0059-skb_xmit_more/skb_no_xmit_more.cocci
@@ -0,0 +1,9 @@
+@r1@
+struct sk_buff *skb;
+expression E1;
+@@
+ if (E1
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0)
+ || !skb->xmit_more
++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,18,0) */
+ ) {...}
--
2.4.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/4] backports: replace igb pfmemalloc patch with semantic patch
2015-06-03 11:25 [PATCH 0/4] backports: more generic ethernet patches Stefan Assmann
` (2 preceding siblings ...)
2015-06-03 11:25 ` [PATCH 3/4] backports: replace igb xmit_more " Stefan Assmann
@ 2015-06-03 11:25 ` Stefan Assmann
2015-06-09 22:15 ` [PATCH 0/4] backports: more generic ethernet patches Luis R. Rodriguez
4 siblings, 0 replies; 13+ messages in thread
From: Stefan Assmann @ 2015-06-03 11:25 UTC (permalink / raw)
To: backports; +Cc: mcgrof, hauke, sassmann
Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
.../network/0049-no-pfmemalloc/igb.patch | 14 --------------
.../network/0049-no-pfmemalloc/no-pfmemalloc.cocci | 9 +++++++++
2 files changed, 9 insertions(+), 14 deletions(-)
delete mode 100644 patches/collateral-evolutions/network/0049-no-pfmemalloc/igb.patch
create mode 100644 patches/collateral-evolutions/network/0049-no-pfmemalloc/no-pfmemalloc.cocci
diff --git a/patches/collateral-evolutions/network/0049-no-pfmemalloc/igb.patch b/patches/collateral-evolutions/network/0049-no-pfmemalloc/igb.patch
deleted file mode 100644
index 53b319a..0000000
--- a/patches/collateral-evolutions/network/0049-no-pfmemalloc/igb.patch
+++ /dev/null
@@ -1,14 +0,0 @@
---- a/drivers/net/ethernet/intel/igb/igb_main.c
-+++ b/drivers/net/ethernet/intel/igb/igb_main.c
-@@ -6578,7 +6578,11 @@ static void igb_reuse_rx_page(struct igb
-
- static inline bool igb_page_is_reserved(struct page *page)
- {
-+#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,6,0)
- return (page_to_nid(page) != numa_mem_id()) || page->pfmemalloc;
-+#else
-+ return (page_to_nid(page) != numa_mem_id());
-+#endif
- }
-
- static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer,
diff --git a/patches/collateral-evolutions/network/0049-no-pfmemalloc/no-pfmemalloc.cocci b/patches/collateral-evolutions/network/0049-no-pfmemalloc/no-pfmemalloc.cocci
new file mode 100644
index 0000000..bbc1201
--- /dev/null
+++ b/patches/collateral-evolutions/network/0049-no-pfmemalloc/no-pfmemalloc.cocci
@@ -0,0 +1,9 @@
+@r1@
+struct page *page;
+expression E1;
+@@
+ return E1
++#if LINUX_VERSION_CODE >= KERNEL_VERSION(3,6,0)
+ || page->pfmemalloc
++#endif /* if LINUX_VERSION_CODE >= KERNEL_VERSION(3,6,0) */
+ ;
--
2.4.2
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 0/4] backports: more generic ethernet patches
2015-06-03 11:25 [PATCH 0/4] backports: more generic ethernet patches Stefan Assmann
` (3 preceding siblings ...)
2015-06-03 11:25 ` [PATCH 4/4] backports: replace igb pfmemalloc " Stefan Assmann
@ 2015-06-09 22:15 ` Luis R. Rodriguez
4 siblings, 0 replies; 13+ messages in thread
From: Luis R. Rodriguez @ 2015-06-09 22:15 UTC (permalink / raw)
To: Stefan Assmann; +Cc: backports@vger.kernel.org, Hauke Mehrtens
On Wed, Jun 3, 2015 at 4:25 AM, Stefan Assmann <sassmann@kpanic.de> wrote:
> Stefan Assmann (4):
> backports: replace igb skb->no_fcs patch with semantic patch
> backports: replace igb ethtool_cmd_mdix patch with semantic patch
> backports: replace igb xmit_more patch with semantic patch
> backports: replace igb pfmemalloc patch with semantic patch
Thanks Stefan! Applied and pushed!
Luis
^ permalink raw reply [flat|nested] 13+ messages in thread