Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
On Fri, Jun 21, 2019 at 11:33:27AM -0700, Joe Perches wrote: > On Fri, 2019-06-21 at 13:12 -0500, Bjorn Helgaas wrote: > > On Fri, Jun 21, 2019 at 12:27 PM Joe Perches wrote: > [] > > > Subsystem specific local PCI #defines without generic > > > naming is poor style and makes treewide grep and > > > refactoring much more difficult. > > > > Don't worry, we have the same objectives. I totally agree that local > > #defines are a bad thing, which is why I proposed this project in the > > first place. > > Hi again Bjorn. > > I didn't know that was your idea. Good idea. > > > I'm just saying that this is a "first-patch" sort of learning project > > and I think it'll avoid some list spamming and discouragement if we > > can figure out the scope and shake out some of the teething problems > > ahead of time. I don't want to end up with multiple versions of > > dozens of little 2-3 patch series posted every week or two. > > Great, that's sensible. > > > I'd rather be able to deal with a whole block of them at one time. > > Also very sensible. > > > > 2: Show that you compiled the object files and verified > > >where possible that there are no object file changes. > > > > Do you have any pointers for the best way to do this? Is it as simple > > as comparing output of "objdump -d"? > > Generically, yes. > > I have a little script that does the equivalent of: > > > make > mv .old > patch -P1 < > make > mv .new > diff -urN <(objdump -d .old) <(objdump -d .new) > > But it's not foolproof as gcc does not guarantee > compilation repeatability. > > And some subsystems Makefiles do not allow per-file > compilation. > Hi Joe, I tried using your specified technique here are the steps I took and the results I got. I built the object file before the patch named it "atl2-old.o" then I built it after the patch, named it "atl2-new.o" then i ran these commands:- $ objdump -d atl2-old.o > 1 $ objdump -d atl2-new.o > 2 $ diff -urN 1 2 --- 1 2019-06-22 13:56:17.881392372 +0530 +++ 2 2019-06-22 13:56:35.228018053 +0530 @@ -1,5 +1,5 @@ -atl2-old.o: file format elf64-x86-64 +atl2-new.o: file format elf64-x86-64 Disassembly of section .text: So both the object files are similar. Thanks, --Puranjay
Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
On Fri, Jun 21, 2019 at 11:33 AM Joe Perches wrote: > > On Fri, 2019-06-21 at 13:12 -0500, Bjorn Helgaas wrote: > > On Fri, Jun 21, 2019 at 12:27 PM Joe Perches wrote: > [] > > > Subsystem specific local PCI #defines without generic > > > naming is poor style and makes treewide grep and > > > refactoring much more difficult. > > > > Don't worry, we have the same objectives. I totally agree that local > > #defines are a bad thing, which is why I proposed this project in the > > first place. > > Hi again Bjorn. > > I didn't know that was your idea. Good idea. > > > I'm just saying that this is a "first-patch" sort of learning project > > and I think it'll avoid some list spamming and discouragement if we > > can figure out the scope and shake out some of the teething problems > > ahead of time. I don't want to end up with multiple versions of > > dozens of little 2-3 patch series posted every week or two. > > Great, that's sensible. > > > I'd rather be able to deal with a whole block of them at one time. > > Also very sensible. > > > > 2: Show that you compiled the object files and verified > > >where possible that there are no object file changes. > > > > Do you have any pointers for the best way to do this? Is it as simple > > as comparing output of "objdump -d"? > > Generically, yes. > > I have a little script that does the equivalent of: > > > make > mv .old > patch -P1 < > make > mv .new > diff -urN <(objdump -d .old) <(objdump -d .new) > > But it's not foolproof as gcc does not guarantee > compilation repeatability. > > And some subsystems Makefiles do not allow per-file > compilation. > This should work, but be aware that the older atlx drivers did some regrettable things with file structure, so not all .c files are expected to generate a corresponding .o file. - Chris
Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
On Fri, 2019-06-21 at 13:12 -0500, Bjorn Helgaas wrote: > On Fri, Jun 21, 2019 at 12:27 PM Joe Perches wrote: [] > > Subsystem specific local PCI #defines without generic > > naming is poor style and makes treewide grep and > > refactoring much more difficult. > > Don't worry, we have the same objectives. I totally agree that local > #defines are a bad thing, which is why I proposed this project in the > first place. Hi again Bjorn. I didn't know that was your idea. Good idea. > I'm just saying that this is a "first-patch" sort of learning project > and I think it'll avoid some list spamming and discouragement if we > can figure out the scope and shake out some of the teething problems > ahead of time. I don't want to end up with multiple versions of > dozens of little 2-3 patch series posted every week or two. Great, that's sensible. > I'd rather be able to deal with a whole block of them at one time. Also very sensible. > > 2: Show that you compiled the object files and verified > >where possible that there are no object file changes. > > Do you have any pointers for the best way to do this? Is it as simple > as comparing output of "objdump -d"? Generically, yes. I have a little script that does the equivalent of: make mv .old patch -P1 < make mv .new diff -urN <(objdump -d .old) <(objdump -d .new) But it's not foolproof as gcc does not guarantee compilation repeatability. And some subsystems Makefiles do not allow per-file compilation.
Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
On Fri, Jun 21, 2019 at 12:27 PM Joe Perches wrote: > > (adding the atlx maintainers to cc) > > On Fri, 2019-06-21 at 12:11 -0500, Bjorn Helgaas wrote: > > On Fri, Jun 21, 2019 at 11:39 AM Puranjay Mohan > > wrote: > > > This patch series removes the private duplicates of PCI definitions in > > > favour of generic definitions defined in pci_regs.h. > > > > > > Puranjay Mohan (3): > > > net: ethernet: atheros: atlx: Rename local PCI defines to generic > > > names > > > net: ethernet: atheros: atlx: Include generic PCI definitions > > > net: ethernet: atheros: atlx: Remove unused and private PCI > > > definitions > > > > > > drivers/net/ethernet/atheros/atlx/atl2.c | 5 +++-- > > > drivers/net/ethernet/atheros/atlx/atl2.h | 2 -- > > > drivers/net/ethernet/atheros/atlx/atlx.h | 1 - > > > 3 files changed, 3 insertions(+), 5 deletions(-) > > > > Let's slow this down a little bit; I'm afraid we're going to overwhelm > > folks. > > I generally disagree. > > Consolidation of these sorts of changes are generally > better done treewide all at once, posted as a series to > a list and maintainers allowing time (weeks to months) > for the specific maintainers to accept them and then > whatever remainder exists reposted and possibly applied > by an overall maintainer (e.g.: Dave M) > > > Before posting more to LKML/netdev, how about we first complete a > > sweep of all the drivers to see what we're getting into. It could be > > that this will end up being more churn than it's worth. > > Also doubtful. > > Subsystem specific local PCI #defines without generic > naming is poor style and makes treewide grep and > refactoring much more difficult. Don't worry, we have the same objectives. I totally agree that local #defines are a bad thing, which is why I proposed this project in the first place. I'm just saying that this is a "first-patch" sort of learning project and I think it'll avoid some list spamming and discouragement if we can figure out the scope and shake out some of the teething problems ahead of time. I don't want to end up with multiple versions of dozens of little 2-3 patch series posted every week or two. I'd rather be able to deal with a whole block of them at one time. > The atlx maintainers should definitely have been cc'd > on these patches. > > Jay Cliburn (maintainer:ATLX ETHERNET DRIVERS) > Chris Snook (maintainer:ATLX ETHERNET DRIVERS) > > Puranjay, can you please do a few things more here: > > 1: Make sure you use scripts/get_maintainer.pl to cc the >appropriate people. > > 2: Show that you compiled the object files and verified >where possible that there are no object file changes. Do you have any pointers for the best way to do this? Is it as simple as comparing output of "objdump -d"? > 3: State that there are no object changes in the proposed >commit log. Thanks for the additional tips. Bjorn
Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
(adding the atlx maintainers to cc) On Fri, 2019-06-21 at 12:11 -0500, Bjorn Helgaas wrote: > On Fri, Jun 21, 2019 at 11:39 AM Puranjay Mohan wrote: > > This patch series removes the private duplicates of PCI definitions in > > favour of generic definitions defined in pci_regs.h. > > > > Puranjay Mohan (3): > > net: ethernet: atheros: atlx: Rename local PCI defines to generic > > names > > net: ethernet: atheros: atlx: Include generic PCI definitions > > net: ethernet: atheros: atlx: Remove unused and private PCI > > definitions > > > > drivers/net/ethernet/atheros/atlx/atl2.c | 5 +++-- > > drivers/net/ethernet/atheros/atlx/atl2.h | 2 -- > > drivers/net/ethernet/atheros/atlx/atlx.h | 1 - > > 3 files changed, 3 insertions(+), 5 deletions(-) > > Let's slow this down a little bit; I'm afraid we're going to overwhelm folks. I generally disagree. Consolidation of these sorts of changes are generally better done treewide all at once, posted as a series to a list and maintainers allowing time (weeks to months) for the specific maintainers to accept them and then whatever remainder exists reposted and possibly applied by an overall maintainer (e.g.: Dave M) > Before posting more to LKML/netdev, how about we first complete a > sweep of all the drivers to see what we're getting into. It could be > that this will end up being more churn than it's worth. Also doubtful. Subsystem specific local PCI #defines without generic naming is poor style and makes treewide grep and refactoring much more difficult. The atlx maintainers should definitely have been cc'd on these patches. Jay Cliburn (maintainer:ATLX ETHERNET DRIVERS) Chris Snook (maintainer:ATLX ETHERNET DRIVERS) Puranjay, can you please do a few things more here: 1: Make sure you use scripts/get_maintainer.pl to cc the appropriate people. 2: Show that you compiled the object files and verified where possible that there are no object file changes. 3: State that there are no object changes in the proposed commit log. thanks.
Re: [PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
On Fri, Jun 21, 2019 at 11:39 AM Puranjay Mohan wrote: > > This patch series removes the private duplicates of PCI definitions in > favour of generic definitions defined in pci_regs.h. > > Puranjay Mohan (3): > net: ethernet: atheros: atlx: Rename local PCI defines to generic > names > net: ethernet: atheros: atlx: Include generic PCI definitions > net: ethernet: atheros: atlx: Remove unused and private PCI > definitions > > drivers/net/ethernet/atheros/atlx/atl2.c | 5 +++-- > drivers/net/ethernet/atheros/atlx/atl2.h | 2 -- > drivers/net/ethernet/atheros/atlx/atlx.h | 1 - > 3 files changed, 3 insertions(+), 5 deletions(-) Let's slow this down a little bit; I'm afraid we're going to overwhelm folks. Before posting more to LKML/netdev, how about we first complete a sweep of all the drivers to see what we're getting into. It could be that this will end up being more churn than it's worth. You could send the result to linux-kernel-mentees, Shuah, me (but not LKML and netdev), and then we can talk about whether it should be posted all at once, as several series, etc.
[PATCH 0/3] net: ethernet: atheros: atlx: Use PCI generic definitions instead of private duplicates
This patch series removes the private duplicates of PCI definitions in favour of generic definitions defined in pci_regs.h. Puranjay Mohan (3): net: ethernet: atheros: atlx: Rename local PCI defines to generic names net: ethernet: atheros: atlx: Include generic PCI definitions net: ethernet: atheros: atlx: Remove unused and private PCI definitions drivers/net/ethernet/atheros/atlx/atl2.c | 5 +++-- drivers/net/ethernet/atheros/atlx/atl2.h | 2 -- drivers/net/ethernet/atheros/atlx/atlx.h | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) -- 2.21.0
