Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On 11/28/2017 02:59 PM, Greg KH wrote: > On Mon, Nov 27, 2017 at 03:32:28PM +, Laurentiu Tudor wrote: >> >> >> On 11/03/2017 05:17 PM, Greg KH wrote: >>> On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > From: Stuart Yoder> > Move the source files out of staging into their final locations: > -include files in drivers/staging/fsl-mc/include go to > include/linux/fsl > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > -README.txt, providing and overview of DPAA goes to > Documentation/dpaa2/overview.txt > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > Update dpaa2_eth and dpio staging drivers. > > Signed-off-by: Stuart Yoder > Signed-off-by: Laurentiu Tudor > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > Cc: Thomas Gleixner > Cc: Jason Cooper > Cc: Marc Zyngier This is going to have to wait until I get a chunk of time to do the review. Probably after 4.13-final is out. >>> >>> Ok, review comments as I go through the code: >>> mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); >>> >>> should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your >>> other exports and global namespace, right? >>> >>> You export a lot of dpcon_* symbols that no one uses, please do not do >>> that. Verify that all of them are actually used right now, if not, >>> remove them. If you think you are going to use them in the future, >>> wonderful, add them in then. >>> >>> Same for your dpaa2_* exported symbols, most are not used from what I >>> can see. >>> >>> struct dpaa2_io { >>> atomic_t refs; >>> >>> That's a kref, please use it instead of trying to roll your own. >>> >>> And even for this, your locking is not correct (i.e. you do not have >>> any), that needs to be fixed so that teardown works correctly. >>> >>> You have a lot of WARN_ON() calls, that's going to be ignored and should >>> all not be needed now that the code is debugged and working properly. >>> Please remove them, or turn them into dev_err() calls with a real if () >>> check instead. >>> >>> You are checking "strings" for the type of device in a lot of places, >>> like this: >>> if (strcmp(obj_desc->type, "dprc") == 0) { >>> why are you not just using the built-in driver model .type field and >>> comparing that to the different type structures? It's much easier, and >>> you don't have to again, "roll your own". See the USB or Greybus code >>> for examples of busses that have different "types" of devices on them at >>> the same time. >>> >>> Ok, that's enough for now, please work on this, and we can go from >>> there... >>> >> >> What would the next steps be, now that the patches are in staging-next? >> Are there plans for a new round of review? > > Send a patch that moves the files you think should be moved at this > point in time, as I'm not quite sure which ones exactly you feel are > ready to go. The initial plan was to strictly move the things needed by this bus driver, thus leaving some source files at a later time. To the point, these files: "drivers/staging/fsl-mc/bus/dpbp*.*" "drivers/staging/fsl-mc/bus/dpcon*.*" and this whole dir: "drivers/staging/fsl-mc/bus/dpio/*". I'll prepare the patch so that it's more visible what files are to be moved. --- Thanks & Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On 11/28/2017 02:59 PM, Greg KH wrote: > On Mon, Nov 27, 2017 at 03:32:28PM +, Laurentiu Tudor wrote: >> >> >> On 11/03/2017 05:17 PM, Greg KH wrote: >>> On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > From: Stuart Yoder > > Move the source files out of staging into their final locations: > -include files in drivers/staging/fsl-mc/include go to > include/linux/fsl > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > -README.txt, providing and overview of DPAA goes to > Documentation/dpaa2/overview.txt > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > Update dpaa2_eth and dpio staging drivers. > > Signed-off-by: Stuart Yoder > Signed-off-by: Laurentiu Tudor > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > Cc: Thomas Gleixner > Cc: Jason Cooper > Cc: Marc Zyngier This is going to have to wait until I get a chunk of time to do the review. Probably after 4.13-final is out. >>> >>> Ok, review comments as I go through the code: >>> mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); >>> >>> should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your >>> other exports and global namespace, right? >>> >>> You export a lot of dpcon_* symbols that no one uses, please do not do >>> that. Verify that all of them are actually used right now, if not, >>> remove them. If you think you are going to use them in the future, >>> wonderful, add them in then. >>> >>> Same for your dpaa2_* exported symbols, most are not used from what I >>> can see. >>> >>> struct dpaa2_io { >>> atomic_t refs; >>> >>> That's a kref, please use it instead of trying to roll your own. >>> >>> And even for this, your locking is not correct (i.e. you do not have >>> any), that needs to be fixed so that teardown works correctly. >>> >>> You have a lot of WARN_ON() calls, that's going to be ignored and should >>> all not be needed now that the code is debugged and working properly. >>> Please remove them, or turn them into dev_err() calls with a real if () >>> check instead. >>> >>> You are checking "strings" for the type of device in a lot of places, >>> like this: >>> if (strcmp(obj_desc->type, "dprc") == 0) { >>> why are you not just using the built-in driver model .type field and >>> comparing that to the different type structures? It's much easier, and >>> you don't have to again, "roll your own". See the USB or Greybus code >>> for examples of busses that have different "types" of devices on them at >>> the same time. >>> >>> Ok, that's enough for now, please work on this, and we can go from >>> there... >>> >> >> What would the next steps be, now that the patches are in staging-next? >> Are there plans for a new round of review? > > Send a patch that moves the files you think should be moved at this > point in time, as I'm not quite sure which ones exactly you feel are > ready to go. The initial plan was to strictly move the things needed by this bus driver, thus leaving some source files at a later time. To the point, these files: "drivers/staging/fsl-mc/bus/dpbp*.*" "drivers/staging/fsl-mc/bus/dpcon*.*" and this whole dir: "drivers/staging/fsl-mc/bus/dpio/*". I'll prepare the patch so that it's more visible what files are to be moved. --- Thanks & Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Mon, Nov 27, 2017 at 03:32:28PM +, Laurentiu Tudor wrote: > > > On 11/03/2017 05:17 PM, Greg KH wrote: > > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: > >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > >>> From: Stuart Yoder> >>> > >>> Move the source files out of staging into their final locations: > >>>-include files in drivers/staging/fsl-mc/include go to > >>> include/linux/fsl > >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > >>>-README.txt, providing and overview of DPAA goes to > >>> Documentation/dpaa2/overview.txt > >>> > >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > >>> Update dpaa2_eth and dpio staging drivers. > >>> > >>> Signed-off-by: Stuart Yoder > >>> Signed-off-by: Laurentiu Tudor > >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > >>> Cc: Thomas Gleixner > >>> Cc: Jason Cooper > >>> Cc: Marc Zyngier > >> > >> This is going to have to wait until I get a chunk of time to do the > >> review. Probably after 4.13-final is out. > > > > Ok, review comments as I go through the code: > > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > > other exports and global namespace, right? > > > > You export a lot of dpcon_* symbols that no one uses, please do not do > > that. Verify that all of them are actually used right now, if not, > > remove them. If you think you are going to use them in the future, > > wonderful, add them in then. > > > > Same for your dpaa2_* exported symbols, most are not used from what I > > can see. > > > > struct dpaa2_io { > > atomic_t refs; > > > > That's a kref, please use it instead of trying to roll your own. > > > > And even for this, your locking is not correct (i.e. you do not have > > any), that needs to be fixed so that teardown works correctly. > > > > You have a lot of WARN_ON() calls, that's going to be ignored and should > > all not be needed now that the code is debugged and working properly. > > Please remove them, or turn them into dev_err() calls with a real if () > > check instead. > > > > You are checking "strings" for the type of device in a lot of places, > > like this: > > if (strcmp(obj_desc->type, "dprc") == 0) { > > why are you not just using the built-in driver model .type field and > > comparing that to the different type structures? It's much easier, and > > you don't have to again, "roll your own". See the USB or Greybus code > > for examples of busses that have different "types" of devices on them at > > the same time. > > > > Ok, that's enough for now, please work on this, and we can go from > > there... > > > > What would the next steps be, now that the patches are in staging-next? > Are there plans for a new round of review? Send a patch that moves the files you think should be moved at this point in time, as I'm not quite sure which ones exactly you feel are ready to go. thanks, greg k-h
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Mon, Nov 27, 2017 at 03:32:28PM +, Laurentiu Tudor wrote: > > > On 11/03/2017 05:17 PM, Greg KH wrote: > > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: > >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > >>> From: Stuart Yoder > >>> > >>> Move the source files out of staging into their final locations: > >>>-include files in drivers/staging/fsl-mc/include go to > >>> include/linux/fsl > >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > >>>-README.txt, providing and overview of DPAA goes to > >>> Documentation/dpaa2/overview.txt > >>> > >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > >>> Update dpaa2_eth and dpio staging drivers. > >>> > >>> Signed-off-by: Stuart Yoder > >>> Signed-off-by: Laurentiu Tudor > >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > >>> Cc: Thomas Gleixner > >>> Cc: Jason Cooper > >>> Cc: Marc Zyngier > >> > >> This is going to have to wait until I get a chunk of time to do the > >> review. Probably after 4.13-final is out. > > > > Ok, review comments as I go through the code: > > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > > other exports and global namespace, right? > > > > You export a lot of dpcon_* symbols that no one uses, please do not do > > that. Verify that all of them are actually used right now, if not, > > remove them. If you think you are going to use them in the future, > > wonderful, add them in then. > > > > Same for your dpaa2_* exported symbols, most are not used from what I > > can see. > > > > struct dpaa2_io { > > atomic_t refs; > > > > That's a kref, please use it instead of trying to roll your own. > > > > And even for this, your locking is not correct (i.e. you do not have > > any), that needs to be fixed so that teardown works correctly. > > > > You have a lot of WARN_ON() calls, that's going to be ignored and should > > all not be needed now that the code is debugged and working properly. > > Please remove them, or turn them into dev_err() calls with a real if () > > check instead. > > > > You are checking "strings" for the type of device in a lot of places, > > like this: > > if (strcmp(obj_desc->type, "dprc") == 0) { > > why are you not just using the built-in driver model .type field and > > comparing that to the different type structures? It's much easier, and > > you don't have to again, "roll your own". See the USB or Greybus code > > for examples of busses that have different "types" of devices on them at > > the same time. > > > > Ok, that's enough for now, please work on this, and we can go from > > there... > > > > What would the next steps be, now that the patches are in staging-next? > Are there plans for a new round of review? Send a patch that moves the files you think should be moved at this point in time, as I'm not quite sure which ones exactly you feel are ready to go. thanks, greg k-h
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: >>> From: Stuart Yoder>>> >>> Move the source files out of staging into their final locations: >>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>>-README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > other exports and global namespace, right? > > You export a lot of dpcon_* symbols that no one uses, please do not do > that. Verify that all of them are actually used right now, if not, > remove them. If you think you are going to use them in the future, > wonderful, add them in then. > > Same for your dpaa2_* exported symbols, most are not used from what I > can see. > > struct dpaa2_io { > atomic_t refs; > > That's a kref, please use it instead of trying to roll your own. > > And even for this, your locking is not correct (i.e. you do not have > any), that needs to be fixed so that teardown works correctly. > > You have a lot of WARN_ON() calls, that's going to be ignored and should > all not be needed now that the code is debugged and working properly. > Please remove them, or turn them into dev_err() calls with a real if () > check instead. > > You are checking "strings" for the type of device in a lot of places, > like this: > if (strcmp(obj_desc->type, "dprc") == 0) { > why are you not just using the built-in driver model .type field and > comparing that to the different type structures? It's much easier, and > you don't have to again, "roll your own". See the USB or Greybus code > for examples of busses that have different "types" of devices on them at > the same time. > > Ok, that's enough for now, please work on this, and we can go from > there... > What would the next steps be, now that the patches are in staging-next? Are there plans for a new round of review? --- Thanks & Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: >>> From: Stuart Yoder >>> >>> Move the source files out of staging into their final locations: >>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>>-README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > other exports and global namespace, right? > > You export a lot of dpcon_* symbols that no one uses, please do not do > that. Verify that all of them are actually used right now, if not, > remove them. If you think you are going to use them in the future, > wonderful, add them in then. > > Same for your dpaa2_* exported symbols, most are not used from what I > can see. > > struct dpaa2_io { > atomic_t refs; > > That's a kref, please use it instead of trying to roll your own. > > And even for this, your locking is not correct (i.e. you do not have > any), that needs to be fixed so that teardown works correctly. > > You have a lot of WARN_ON() calls, that's going to be ignored and should > all not be needed now that the code is debugged and working properly. > Please remove them, or turn them into dev_err() calls with a real if () > check instead. > > You are checking "strings" for the type of device in a lot of places, > like this: > if (strcmp(obj_desc->type, "dprc") == 0) { > why are you not just using the built-in driver model .type field and > comparing that to the different type structures? It's much easier, and > you don't have to again, "roll your own". See the USB or Greybus code > for examples of busses that have different "types" of devices on them at > the same time. > > Ok, that's enough for now, please work on this, and we can go from > there... > What would the next steps be, now that the patches are in staging-next? Are there plans for a new round of review? --- Thanks & Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
>> struct dpaa2_io { >> atomic_t refs; >> >> That's a kref, please use it instead of trying to roll your own. >> >> And even for this, your locking is not correct (i.e. you do not have >> any), that needs to be fixed so that teardown works correctly. > > I think we can drop this refcount altogether as it's not used. Roy, any > comment on this? > Yes I think this refcount can be removed. I'll make a note for when the DPIO is moving out of staging but that isn't part of this patchset. There are other cleanups needed in DPIO as well. I've been holding off on pushing patches for that until the bus driver gets moved to try to avoid complex patch dependencies and merge conflict confusion.
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
>> struct dpaa2_io { >> atomic_t refs; >> >> That's a kref, please use it instead of trying to roll your own. >> >> And even for this, your locking is not correct (i.e. you do not have >> any), that needs to be fixed so that teardown works correctly. > > I think we can drop this refcount altogether as it's not used. Roy, any > comment on this? > Yes I think this refcount can be removed. I'll make a note for when the DPIO is moving out of staging but that isn't part of this patchset. There are other cleanups needed in DPIO as well. I've been holding off on pushing patches for that until the bus driver gets moved to try to avoid complex patch dependencies and merge conflict confusion.
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: >>> From: Stuart Yoder>>> >>> Move the source files out of staging into their final locations: >>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>>-README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > other exports and global namespace, right? Right. There's an inconsistent mixture of EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL() usage. I'll change them all to EXPORT_SYMBOL_GPL(). > You export a lot of dpcon_* symbols that no one uses, please do not do > that. Verify that all of them are actually used right now, if not, > remove them. If you think you are going to use them in the future, > wonderful, add them in then. Actually, most of the dpcon_* APIs are used in the ethernet driver here: drivers/staging/fsl-dpaa2/ethernet. I think i saw only a couple of functions that are not used so I'll drop those. > Same for your dpaa2_* exported symbols, most are not used from what I > can see. I'll check these too and drop the unused ones. > struct dpaa2_io { > atomic_t refs; > > That's a kref, please use it instead of trying to roll your own. > > And even for this, your locking is not correct (i.e. you do not have > any), that needs to be fixed so that teardown works correctly. I think we can drop this refcount altogether as it's not used. Roy, any comment on this? > You have a lot of WARN_ON() calls, that's going to be ignored and should > all not be needed now that the code is debugged and working properly. > Please remove them, or turn them into dev_err() calls with a real if () > check instead. Right, there are quite of few (100+) WARN_ON()s. I'll go through them and clean them up ... most of them seem paranoid checks anyway. > You are checking "strings" for the type of device in a lot of places, > like this: > if (strcmp(obj_desc->type, "dprc") == 0) { > why are you not just using the built-in driver model .type field and > comparing that to the different type structures? It's much easier, and > you don't have to again, "roll your own". See the USB or Greybus code > for examples of busses that have different "types" of devices on them at > the same time. > I had a quick look over greybus and noticed the device types declared in drivers/staging/greybus/greybus.h plus some helper macros to check the device type. I'll give this a go, though i might return with some questions (e.g. I don't yet understand what device_type::release op is used for). --- Thanks & Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: >>> From: Stuart Yoder >>> >>> Move the source files out of staging into their final locations: >>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>>-README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: > mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); > > should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your > other exports and global namespace, right? Right. There's an inconsistent mixture of EXPORT_SYMBOL() and EXPORT_SYMBOL_GPL() usage. I'll change them all to EXPORT_SYMBOL_GPL(). > You export a lot of dpcon_* symbols that no one uses, please do not do > that. Verify that all of them are actually used right now, if not, > remove them. If you think you are going to use them in the future, > wonderful, add them in then. Actually, most of the dpcon_* APIs are used in the ethernet driver here: drivers/staging/fsl-dpaa2/ethernet. I think i saw only a couple of functions that are not used so I'll drop those. > Same for your dpaa2_* exported symbols, most are not used from what I > can see. I'll check these too and drop the unused ones. > struct dpaa2_io { > atomic_t refs; > > That's a kref, please use it instead of trying to roll your own. > > And even for this, your locking is not correct (i.e. you do not have > any), that needs to be fixed so that teardown works correctly. I think we can drop this refcount altogether as it's not used. Roy, any comment on this? > You have a lot of WARN_ON() calls, that's going to be ignored and should > all not be needed now that the code is debugged and working properly. > Please remove them, or turn them into dev_err() calls with a real if () > check instead. Right, there are quite of few (100+) WARN_ON()s. I'll go through them and clean them up ... most of them seem paranoid checks anyway. > You are checking "strings" for the type of device in a lot of places, > like this: > if (strcmp(obj_desc->type, "dprc") == 0) { > why are you not just using the built-in driver model .type field and > comparing that to the different type structures? It's much easier, and > you don't have to again, "roll your own". See the USB or Greybus code > for examples of busses that have different "types" of devices on them at > the same time. > I had a quick look over greybus and noticed the device types declared in drivers/staging/greybus/greybus.h plus some helper macros to check the device type. I'll give this a go, though i might return with some questions (e.g. I don't yet understand what device_type::release op is used for). --- Thanks & Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
Hi Greg, On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: >>> From: Stuart Yoder>>> >>> Move the source files out of staging into their final locations: >>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>>-README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: Thanks a lot for taking the time. I'll take care of the comments next week. --- Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
Hi Greg, On 11/03/2017 05:17 PM, Greg KH wrote: > On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: >> On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: >>> From: Stuart Yoder >>> >>> Move the source files out of staging into their final locations: >>>-include files in drivers/staging/fsl-mc/include go to include/linux/fsl >>>-irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip >>>-source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc >>>-README.txt, providing and overview of DPAA goes to >>> Documentation/dpaa2/overview.txt >>> >>> Update or delete other remaining staging files-- Makefile, Kconfig, TODO. >>> Update dpaa2_eth and dpio staging drivers. >>> >>> Signed-off-by: Stuart Yoder >>> Signed-off-by: Laurentiu Tudor >>> [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] >>> Cc: Thomas Gleixner >>> Cc: Jason Cooper >>> Cc: Marc Zyngier >> >> This is going to have to wait until I get a chunk of time to do the >> review. Probably after 4.13-final is out. > > Ok, review comments as I go through the code: Thanks a lot for taking the time. I'll take care of the comments next week. --- Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > > From: Stuart Yoder> > > > Move the source files out of staging into their final locations: > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > -README.txt, providing and overview of DPAA goes to > >Documentation/dpaa2/overview.txt > > > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > > Update dpaa2_eth and dpio staging drivers. > > > > Signed-off-by: Stuart Yoder > > Signed-off-by: Laurentiu Tudor > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > Cc: Thomas Gleixner > > Cc: Jason Cooper > > Cc: Marc Zyngier > > This is going to have to wait until I get a chunk of time to do the > review. Probably after 4.13-final is out. Ok, review comments as I go through the code: mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your other exports and global namespace, right? You export a lot of dpcon_* symbols that no one uses, please do not do that. Verify that all of them are actually used right now, if not, remove them. If you think you are going to use them in the future, wonderful, add them in then. Same for your dpaa2_* exported symbols, most are not used from what I can see. struct dpaa2_io { atomic_t refs; That's a kref, please use it instead of trying to roll your own. And even for this, your locking is not correct (i.e. you do not have any), that needs to be fixed so that teardown works correctly. You have a lot of WARN_ON() calls, that's going to be ignored and should all not be needed now that the code is debugged and working properly. Please remove them, or turn them into dev_err() calls with a real if () check instead. You are checking "strings" for the type of device in a lot of places, like this: if (strcmp(obj_desc->type, "dprc") == 0) { why are you not just using the built-in driver model .type field and comparing that to the different type structures? It's much easier, and you don't have to again, "roll your own". See the USB or Greybus code for examples of busses that have different "types" of devices on them at the same time. Ok, that's enough for now, please work on this, and we can go from there... thanks, greg k-h
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Thu, Aug 31, 2017 at 06:04:30PM +0200, Greg KH wrote: > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > > From: Stuart Yoder > > > > Move the source files out of staging into their final locations: > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > -README.txt, providing and overview of DPAA goes to > >Documentation/dpaa2/overview.txt > > > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > > Update dpaa2_eth and dpio staging drivers. > > > > Signed-off-by: Stuart Yoder > > Signed-off-by: Laurentiu Tudor > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > Cc: Thomas Gleixner > > Cc: Jason Cooper > > Cc: Marc Zyngier > > This is going to have to wait until I get a chunk of time to do the > review. Probably after 4.13-final is out. Ok, review comments as I go through the code: mc-sys.c 323 EXPORT_SYMBOL(mc_send_command); should be EXPORT_SYMBOL_GPL(fsl_mc_send_command); to match up with your other exports and global namespace, right? You export a lot of dpcon_* symbols that no one uses, please do not do that. Verify that all of them are actually used right now, if not, remove them. If you think you are going to use them in the future, wonderful, add them in then. Same for your dpaa2_* exported symbols, most are not used from what I can see. struct dpaa2_io { atomic_t refs; That's a kref, please use it instead of trying to roll your own. And even for this, your locking is not correct (i.e. you do not have any), that needs to be fixed so that teardown works correctly. You have a lot of WARN_ON() calls, that's going to be ignored and should all not be needed now that the code is debugged and working properly. Please remove them, or turn them into dev_err() calls with a real if () check instead. You are checking "strings" for the type of device in a lot of places, like this: if (strcmp(obj_desc->type, "dprc") == 0) { why are you not just using the built-in driver model .type field and comparing that to the different type structures? It's much easier, and you don't have to again, "roll your own". See the USB or Greybus code for examples of busses that have different "types" of devices on them at the same time. Ok, that's enough for now, please work on this, and we can go from there... thanks, greg k-h
RE: [PATCH v2] staging: fsl-mc: move bus driver out of staging
> -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Monday, September 11, 2017 4:03 PM > > On Mon, Sep 11, 2017 at 11:55:22AM +, Laurentiu Tudor wrote: > > Hi, > > > > > -Original Message- > > > From: Greg KH [mailto:gre...@linuxfoundation.org] > > > Sent: Thursday, August 31, 2017 7:05 PM > > > > > > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com > wrote: > > > > From: Stuart Yoder> > > > > > > > Move the source files out of staging into their final locations: > > > > -include files in drivers/staging/fsl-mc/include go to > > > > include/linux/fsl > > > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > > > -README.txt, providing and overview of DPAA goes to > > > >Documentation/dpaa2/overview.txt > > > > > > > > Update or delete other remaining staging files-- Makefile, Kconfig, > > > > TODO. > > > > Update dpaa2_eth and dpio staging drivers. > > > > > > > > Signed-off-by: Stuart Yoder > > > > Signed-off-by: Laurentiu Tudor > > > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > > > Cc: Thomas Gleixner > > > > Cc: Jason Cooper > > > > Cc: Marc Zyngier > > > > > > This is going to have to wait until I get a chunk of time to do the > > > review. > > > Probably after 4.13-final is out. > > > > Any updates on this? Any chances to make it in 4.14? Thanks! > > No, it's too late for 4.14, it would have had to be in linux-next before > 4.13-final > came out. I'm at a conference all this week, Plumbers? :-) > if I'm stuck in a boring talk, I'll try to do the review :) Great. Thanks a lot for taking the time! --- Best Regards, Laurentiu
RE: [PATCH v2] staging: fsl-mc: move bus driver out of staging
> -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Monday, September 11, 2017 4:03 PM > > On Mon, Sep 11, 2017 at 11:55:22AM +, Laurentiu Tudor wrote: > > Hi, > > > > > -Original Message- > > > From: Greg KH [mailto:gre...@linuxfoundation.org] > > > Sent: Thursday, August 31, 2017 7:05 PM > > > > > > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com > wrote: > > > > From: Stuart Yoder > > > > > > > > Move the source files out of staging into their final locations: > > > > -include files in drivers/staging/fsl-mc/include go to > > > > include/linux/fsl > > > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > > > -README.txt, providing and overview of DPAA goes to > > > >Documentation/dpaa2/overview.txt > > > > > > > > Update or delete other remaining staging files-- Makefile, Kconfig, > > > > TODO. > > > > Update dpaa2_eth and dpio staging drivers. > > > > > > > > Signed-off-by: Stuart Yoder > > > > Signed-off-by: Laurentiu Tudor > > > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > > > Cc: Thomas Gleixner > > > > Cc: Jason Cooper > > > > Cc: Marc Zyngier > > > > > > This is going to have to wait until I get a chunk of time to do the > > > review. > > > Probably after 4.13-final is out. > > > > Any updates on this? Any chances to make it in 4.14? Thanks! > > No, it's too late for 4.14, it would have had to be in linux-next before > 4.13-final > came out. I'm at a conference all this week, Plumbers? :-) > if I'm stuck in a boring talk, I'll try to do the review :) Great. Thanks a lot for taking the time! --- Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Mon, Sep 11, 2017 at 11:55:22AM +, Laurentiu Tudor wrote: > Hi, > > > -Original Message- > > From: Greg KH [mailto:gre...@linuxfoundation.org] > > Sent: Thursday, August 31, 2017 7:05 PM > > > > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > > > From: Stuart Yoder> > > > > > Move the source files out of staging into their final locations: > > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > > -README.txt, providing and overview of DPAA goes to > > >Documentation/dpaa2/overview.txt > > > > > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > > > Update dpaa2_eth and dpio staging drivers. > > > > > > Signed-off-by: Stuart Yoder > > > Signed-off-by: Laurentiu Tudor > > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > > Cc: Thomas Gleixner > > > Cc: Jason Cooper > > > Cc: Marc Zyngier > > > > This is going to have to wait until I get a chunk of time to do the review. > > Probably after 4.13-final is out. > > Any updates on this? Any chances to make it in 4.14? Thanks! No, it's too late for 4.14, it would have had to be in linux-next before 4.13-final came out. I'm at a conference all this week, if I'm stuck in a boring talk, I'll try to do the review :) thanks, greg k-h
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Mon, Sep 11, 2017 at 11:55:22AM +, Laurentiu Tudor wrote: > Hi, > > > -Original Message- > > From: Greg KH [mailto:gre...@linuxfoundation.org] > > Sent: Thursday, August 31, 2017 7:05 PM > > > > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > > > From: Stuart Yoder > > > > > > Move the source files out of staging into their final locations: > > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > > -README.txt, providing and overview of DPAA goes to > > >Documentation/dpaa2/overview.txt > > > > > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > > > Update dpaa2_eth and dpio staging drivers. > > > > > > Signed-off-by: Stuart Yoder > > > Signed-off-by: Laurentiu Tudor > > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > > Cc: Thomas Gleixner > > > Cc: Jason Cooper > > > Cc: Marc Zyngier > > > > This is going to have to wait until I get a chunk of time to do the review. > > Probably after 4.13-final is out. > > Any updates on this? Any chances to make it in 4.14? Thanks! No, it's too late for 4.14, it would have had to be in linux-next before 4.13-final came out. I'm at a conference all this week, if I'm stuck in a boring talk, I'll try to do the review :) thanks, greg k-h
RE: [PATCH v2] staging: fsl-mc: move bus driver out of staging
Hi, > -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Thursday, August 31, 2017 7:05 PM > > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > > From: Stuart Yoder> > > > Move the source files out of staging into their final locations: > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > -README.txt, providing and overview of DPAA goes to > >Documentation/dpaa2/overview.txt > > > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > > Update dpaa2_eth and dpio staging drivers. > > > > Signed-off-by: Stuart Yoder > > Signed-off-by: Laurentiu Tudor > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > Cc: Thomas Gleixner > > Cc: Jason Cooper > > Cc: Marc Zyngier > > This is going to have to wait until I get a chunk of time to do the review. > Probably after 4.13-final is out. Any updates on this? Any chances to make it in 4.14? Thanks! --- Best Regards, Laurentiu
RE: [PATCH v2] staging: fsl-mc: move bus driver out of staging
Hi, > -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Thursday, August 31, 2017 7:05 PM > > On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > > From: Stuart Yoder > > > > Move the source files out of staging into their final locations: > > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > > -README.txt, providing and overview of DPAA goes to > >Documentation/dpaa2/overview.txt > > > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > > Update dpaa2_eth and dpio staging drivers. > > > > Signed-off-by: Stuart Yoder > > Signed-off-by: Laurentiu Tudor > > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > > Cc: Thomas Gleixner > > Cc: Jason Cooper > > Cc: Marc Zyngier > > This is going to have to wait until I get a chunk of time to do the review. > Probably after 4.13-final is out. Any updates on this? Any chances to make it in 4.14? Thanks! --- Best Regards, Laurentiu
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > From: Stuart Yoder> > Move the source files out of staging into their final locations: > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > -README.txt, providing and overview of DPAA goes to >Documentation/dpaa2/overview.txt > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > Update dpaa2_eth and dpio staging drivers. > > Signed-off-by: Stuart Yoder > Signed-off-by: Laurentiu Tudor > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > Cc: Thomas Gleixner > Cc: Jason Cooper > Cc: Marc Zyngier This is going to have to wait until I get a chunk of time to do the review. Probably after 4.13-final is out. thanks, greg k-h
Re: [PATCH v2] staging: fsl-mc: move bus driver out of staging
On Mon, Aug 28, 2017 at 01:54:05PM +0300, laurentiu.tu...@nxp.com wrote: > From: Stuart Yoder > > Move the source files out of staging into their final locations: > -include files in drivers/staging/fsl-mc/include go to include/linux/fsl > -irq-gic-v3-its-fsl-mc-msi.c goes to drivers/irqchip > -source in drivers/staging/fsl-mc/bus goes to drivers/bus/fsl-mc > -README.txt, providing and overview of DPAA goes to >Documentation/dpaa2/overview.txt > > Update or delete other remaining staging files-- Makefile, Kconfig, TODO. > Update dpaa2_eth and dpio staging drivers. > > Signed-off-by: Stuart Yoder > Signed-off-by: Laurentiu Tudor > [Laurentiu: rebased, add dpaa2_eth and dpio #include updates] > Cc: Thomas Gleixner > Cc: Jason Cooper > Cc: Marc Zyngier This is going to have to wait until I get a chunk of time to do the review. Probably after 4.13-final is out. thanks, greg k-h