backports: more generic ethernet patches

Signed-off-by Stefan Assmann <[email protected]>

---
 .../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

patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch deleted +0 -14

deleted file mode 100644
index f659bfd..0000000
@@ -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;
- }
- 

patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci new file +0 -0

new file mode 100644
index 0000000..703c227

Quoted 35 lines · Stefan Assmann

Signed-off-by Stefan Assmann <[email protected]>

---
 .../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

patches/collateral-evolutions/network/0035-skb_no_fcs/igb_skb_no_fcs.patch deleted +0 -14

deleted file mode 100644
index f659bfd..0000000
@@ -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;
- }
- 

patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci new file +0 -0

new file mode 100644
index 0000000..703c227

patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci line 7

Quoted 7 lines

patches/collateral-evolutions/network/0035-skb_no_fcs/skb_no_fcs.cocci new file +7 -0

@@ -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

Quoted 3 lines · Hauke Mehrtens

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.

Quoted 5 lines · Stefan Assmann

Quoted 3 lines · Hauke Mehrtens

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.

Quoted 8 lines

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.

Quoted 33 lines · Luis R. Rodriguez

Quoted 5 lines · Stefan Assmann

Quoted 3 lines · Hauke Mehrtens

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.

Quoted 8 lines

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.

Quoted 5 lines

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?

Quoted 40 lines · Stefan Assmann

Quoted 33 lines · Luis R. Rodriguez

Quoted 5 lines · Stefan Assmann

Quoted 3 lines · Hauke Mehrtens

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.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 60 lines · Luis R. Rodriguez

Quoted 40 lines · Stefan Assmann

Quoted 33 lines · Luis R. Rodriguez

Quoted 5 lines · Stefan Assmann

Quoted 3 lines · Hauke Mehrtens

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.

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 7 lines

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.

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