Re: [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers
On Sun, Oct 18, 2020 at 08:02:53PM +0900, Benjamin Poirier wrote: On 2020-10-17 07:16 +0800, Coiby Xu wrote: On Thu, Oct 15, 2020 at 10:01:36AM +0900, Benjamin Poirier wrote: > On 2020-10-14 18:43 +0800, Coiby Xu wrote: > > To avoid namespace clashes with other qlogic drivers and also for the > > sake of naming consistency, use the "qlge_" prefix as suggested in > > drivers/staging/qlge/TODO. > > > > Suggested-by: Benjamin Poirier > > Signed-off-by: Coiby Xu > > --- > > drivers/staging/qlge/TODO |4 - > > drivers/staging/qlge/qlge.h | 190 ++-- > > drivers/staging/qlge/qlge_dbg.c | 1073 --- > > drivers/staging/qlge/qlge_ethtool.c | 231 ++--- > > drivers/staging/qlge/qlge_main.c| 1257 +-- > > drivers/staging/qlge/qlge_mpi.c | 352 > > 6 files changed, 1551 insertions(+), 1556 deletions(-) > > > > diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO > > index f93f7428f5d5..5ac55664c3e2 100644 > > --- a/drivers/staging/qlge/TODO > > +++ b/drivers/staging/qlge/TODO > > @@ -28,10 +28,6 @@ > > * the driver has a habit of using runtime checks where compile time checks are > >possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers()) > > * reorder struct members to avoid holes if it doesn't impact performance > > -* in terms of namespace, the driver uses either qlge_, ql_ (used by > > - other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with > > - clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" > > - prefix. > > You only renamed ql -> qlge. The prefix needs to be added where there is > currently none like the second example of that text. On second thoughts, these structs like ob_mac_iocb_req are defined in local headers and there is no mixed usage. So even when we want to build this diver and other qlogic drivers into the kernel instead of as separate modules, it won't lead to real problems, is it right? Using cscope or ctags and searching for ob_mac_iocb_req will yield ambiguous results. It might also make things more difficult if using a debugger. Even if I have been using gtags, I didn't realize it. Thank you for explaining it to me. Looking at other drivers (ex. ice, mlx5), they use a prefix for their private types, just like for their static functions, even though it's not absolutely necessary. I think it's helpful when reading the code because it quickly shows that it is something that was defined in the driver, not in some subsystem. Good point! I didn't think about it earlier but it would have been better to leave out this renaming to a subsequent patchset, since this change is unrelated to the debugging features. It seems it's more reasonable to do renaming first. So in a sense, the renaming is a preparatory step for the debugging features. -- Best regards, Coiby
Re: [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers
On 2020-10-17 07:16 +0800, Coiby Xu wrote: > On Thu, Oct 15, 2020 at 10:01:36AM +0900, Benjamin Poirier wrote: > > On 2020-10-14 18:43 +0800, Coiby Xu wrote: > > > To avoid namespace clashes with other qlogic drivers and also for the > > > sake of naming consistency, use the "qlge_" prefix as suggested in > > > drivers/staging/qlge/TODO. > > > > > > Suggested-by: Benjamin Poirier > > > Signed-off-by: Coiby Xu > > > --- > > > drivers/staging/qlge/TODO |4 - > > > drivers/staging/qlge/qlge.h | 190 ++-- > > > drivers/staging/qlge/qlge_dbg.c | 1073 --- > > > drivers/staging/qlge/qlge_ethtool.c | 231 ++--- > > > drivers/staging/qlge/qlge_main.c| 1257 +-- > > > drivers/staging/qlge/qlge_mpi.c | 352 > > > 6 files changed, 1551 insertions(+), 1556 deletions(-) > > > > > > diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO > > > index f93f7428f5d5..5ac55664c3e2 100644 > > > --- a/drivers/staging/qlge/TODO > > > +++ b/drivers/staging/qlge/TODO > > > @@ -28,10 +28,6 @@ > > > * the driver has a habit of using runtime checks where compile time > > > checks are > > >possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers()) > > > * reorder struct members to avoid holes if it doesn't impact performance > > > -* in terms of namespace, the driver uses either qlge_, ql_ (used by > > > - other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing > > > (with > > > - clashes, ex: struct ob_mac_iocb_req). Rename everything to use the > > > "qlge_" > > > - prefix. > > > > You only renamed ql -> qlge. The prefix needs to be added where there is > > currently none like the second example of that text. > > On second thoughts, these structs like ob_mac_iocb_req are defined in > local headers and there is no mixed usage. So even when we want to > build this diver and other qlogic drivers into the kernel instead of > as separate modules, it won't lead to real problems, is it right? Using cscope or ctags and searching for ob_mac_iocb_req will yield ambiguous results. It might also make things more difficult if using a debugger. Looking at other drivers (ex. ice, mlx5), they use a prefix for their private types, just like for their static functions, even though it's not absolutely necessary. I think it's helpful when reading the code because it quickly shows that it is something that was defined in the driver, not in some subsystem. I didn't think about it earlier but it would have been better to leave out this renaming to a subsequent patchset, since this change is unrelated to the debugging features.
Re: [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers
On Thu, Oct 15, 2020 at 10:01:36AM +0900, Benjamin Poirier wrote: On 2020-10-14 18:43 +0800, Coiby Xu wrote: To avoid namespace clashes with other qlogic drivers and also for the sake of naming consistency, use the "qlge_" prefix as suggested in drivers/staging/qlge/TODO. Suggested-by: Benjamin Poirier Signed-off-by: Coiby Xu --- drivers/staging/qlge/TODO |4 - drivers/staging/qlge/qlge.h | 190 ++-- drivers/staging/qlge/qlge_dbg.c | 1073 --- drivers/staging/qlge/qlge_ethtool.c | 231 ++--- drivers/staging/qlge/qlge_main.c| 1257 +-- drivers/staging/qlge/qlge_mpi.c | 352 6 files changed, 1551 insertions(+), 1556 deletions(-) diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO index f93f7428f5d5..5ac55664c3e2 100644 --- a/drivers/staging/qlge/TODO +++ b/drivers/staging/qlge/TODO @@ -28,10 +28,6 @@ * the driver has a habit of using runtime checks where compile time checks are possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers()) * reorder struct members to avoid holes if it doesn't impact performance -* in terms of namespace, the driver uses either qlge_, ql_ (used by - other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with - clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" - prefix. You only renamed ql -> qlge. The prefix needs to be added where there is currently none like the second example of that text. On second thoughts, these structs like ob_mac_iocb_req are defined in local headers and there is no mixed usage. So even when we want to build this diver and other qlogic drivers into the kernel instead of as separate modules, it won't lead to real problems, is it right? Besides, the next patch reintroduces the name struct ql_adapter. -- Best regards, Coiby
Re: [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers
On Thu, Oct 15, 2020 at 12:26:28PM +0800, Coiby Xu wrote: On Thu, Oct 15, 2020 at 10:01:36AM +0900, Benjamin Poirier wrote: On 2020-10-14 18:43 +0800, Coiby Xu wrote: To avoid namespace clashes with other qlogic drivers and also for the sake of naming consistency, use the "qlge_" prefix as suggested in drivers/staging/qlge/TODO. Suggested-by: Benjamin Poirier Signed-off-by: Coiby Xu --- drivers/staging/qlge/TODO |4 - drivers/staging/qlge/qlge.h | 190 ++-- drivers/staging/qlge/qlge_dbg.c | 1073 --- drivers/staging/qlge/qlge_ethtool.c | 231 ++--- drivers/staging/qlge/qlge_main.c| 1257 +-- drivers/staging/qlge/qlge_mpi.c | 352 6 files changed, 1551 insertions(+), 1556 deletions(-) diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO index f93f7428f5d5..5ac55664c3e2 100644 --- a/drivers/staging/qlge/TODO +++ b/drivers/staging/qlge/TODO @@ -28,10 +28,6 @@ * the driver has a habit of using runtime checks where compile time checks are possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers()) * reorder struct members to avoid holes if it doesn't impact performance -* in terms of namespace, the driver uses either qlge_, ql_ (used by - other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with - clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" - prefix. You only renamed ql -> qlge. The prefix needs to be added where there is currently none like the second example of that text. Thank you for reminding me of the second example! Besides, the next patch reintroduces the name struct ql_adapter. Oh, there is still a left-over ql_adapter in qlge.h (I renamed ql->qlge after initializing the devlink framework earlier but did a git rebase to make the order of the changes more reasonable). Thank you for the reminding! Btw, is there a way to configure kernel building to let the compiler discover this kind of issue automatically? -- Best regards, Coiby -- Best regards, Coiby
Re: [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers
On Thu, Oct 15, 2020 at 10:01:36AM +0900, Benjamin Poirier wrote: On 2020-10-14 18:43 +0800, Coiby Xu wrote: To avoid namespace clashes with other qlogic drivers and also for the sake of naming consistency, use the "qlge_" prefix as suggested in drivers/staging/qlge/TODO. Suggested-by: Benjamin Poirier Signed-off-by: Coiby Xu --- drivers/staging/qlge/TODO |4 - drivers/staging/qlge/qlge.h | 190 ++-- drivers/staging/qlge/qlge_dbg.c | 1073 --- drivers/staging/qlge/qlge_ethtool.c | 231 ++--- drivers/staging/qlge/qlge_main.c| 1257 +-- drivers/staging/qlge/qlge_mpi.c | 352 6 files changed, 1551 insertions(+), 1556 deletions(-) diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO index f93f7428f5d5..5ac55664c3e2 100644 --- a/drivers/staging/qlge/TODO +++ b/drivers/staging/qlge/TODO @@ -28,10 +28,6 @@ * the driver has a habit of using runtime checks where compile time checks are possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers()) * reorder struct members to avoid holes if it doesn't impact performance -* in terms of namespace, the driver uses either qlge_, ql_ (used by - other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with - clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" - prefix. You only renamed ql -> qlge. The prefix needs to be added where there is currently none like the second example of that text. Thank you for reminding me of the second example! Besides, the next patch reintroduces the name struct ql_adapter. Oh, there is still a left-over ql_adapter in qlge.h (I renamed ql->qlge after initializing the devlink framework earlier but did a git rebase to make the order of the changes more reasonable). Thank you for the reminding! -- Best regards, Coiby
Re: [PATCH v2 1/7] staging: qlge: replace ql_* with qlge_* to avoid namespace clashes with other qlogic drivers
On 2020-10-14 18:43 +0800, Coiby Xu wrote: > To avoid namespace clashes with other qlogic drivers and also for the > sake of naming consistency, use the "qlge_" prefix as suggested in > drivers/staging/qlge/TODO. > > Suggested-by: Benjamin Poirier > Signed-off-by: Coiby Xu > --- > drivers/staging/qlge/TODO |4 - > drivers/staging/qlge/qlge.h | 190 ++-- > drivers/staging/qlge/qlge_dbg.c | 1073 --- > drivers/staging/qlge/qlge_ethtool.c | 231 ++--- > drivers/staging/qlge/qlge_main.c| 1257 +-- > drivers/staging/qlge/qlge_mpi.c | 352 > 6 files changed, 1551 insertions(+), 1556 deletions(-) > > diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO > index f93f7428f5d5..5ac55664c3e2 100644 > --- a/drivers/staging/qlge/TODO > +++ b/drivers/staging/qlge/TODO > @@ -28,10 +28,6 @@ > * the driver has a habit of using runtime checks where compile time checks > are >possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers()) > * reorder struct members to avoid holes if it doesn't impact performance > -* in terms of namespace, the driver uses either qlge_, ql_ (used by > - other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with > - clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_" > - prefix. You only renamed ql -> qlge. The prefix needs to be added where there is currently none like the second example of that text. Besides, the next patch reintroduces the name struct ql_adapter.
