Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Tue, Apr 28, 2015 at 09:52:32PM +0200, Boris Brezillon wrote: > > In particular, I'd like to discuss the threaded-irq approach taken in > this driver (other drivers are using tasklets). > The main reason behind this choice is the fact that crypto engines > are quite fast, and I'm worried about the CPU contention that might > happen in case of fully loaded crypto engines (the CPU might spend most > of its time in interrupt context until the crypto queue is emptied). > Using threaded-irq allows preemption of the crypto completion > operation, thus authorizing another thread (with higher priority) to be > scheduled. ITOH, the tasklet approach provide slightly performances (I > don't recall the exact numbers, but Arnaud did some tests). Either approach is fine with me. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Tue, Apr 28, 2015 at 09:52:32PM +0200, Boris Brezillon wrote: In particular, I'd like to discuss the threaded-irq approach taken in this driver (other drivers are using tasklets). The main reason behind this choice is the fact that crypto engines are quite fast, and I'm worried about the CPU contention that might happen in case of fully loaded crypto engines (the CPU might spend most of its time in interrupt context until the crypto queue is emptied). Using threaded-irq allows preemption of the crypto completion operation, thus authorizing another thread (with higher priority) to be scheduled. ITOH, the tasklet approach provide slightly performances (I don't recall the exact numbers, but Arnaud did some tests). Either approach is fine with me. Cheers, -- Email: Herbert Xu herb...@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Herbert, David, Any comment on the crypto driver implementation ? I've had several reviews focused on: 1/ splitting the patch series into smaller subsets 2/ allowing for smoother transition from the old driver to the new one I'll address (or have addressed) all of these comments, but I'd like to have your opinion on the crypto driver itself. In particular, I'd like to discuss the threaded-irq approach taken in this driver (other drivers are using tasklets). The main reason behind this choice is the fact that crypto engines are quite fast, and I'm worried about the CPU contention that might happen in case of fully loaded crypto engines (the CPU might spend most of its time in interrupt context until the crypto queue is emptied). Using threaded-irq allows preemption of the crypto completion operation, thus authorizing another thread (with higher priority) to be scheduled. ITOH, the tasklet approach provide slightly performances (I don't recall the exact numbers, but Arnaud did some tests). On Thu, 9 Apr 2015 16:58:41 +0200 Boris Brezillon wrote: > Hello, > > This is an attempt to replace the mv_cesa driver by a new one to address > some limitations of the existing driver. > From a performance and CPU load point of view the most important > limitation is the lack of DMA support, thus preventing us from chaining > crypto operations. > > I know we usually try to adapt existing drivers instead of replacing them > by new ones, but after trying to refactor the mv_cesa driver I realized it > would take longer than writing an new one from scratch. > > Here are the main features brought by this new driver: > - support for armada SoCs (up to 38x) while keeping support for older ones > (Orion and Kirkwood) > - DMA mode to offload the CPU in case of intensive crypto usage > - new algorithms: SHA256, DES and 3DES > > I'd like to thank Arnaud, who has carefully reviewed several iterations of > this driver, helped me improved my implementation, provided support for > several crypto algorithms, provided support for armada-370 and tested > the driver on different platforms, hence the SoB and dual MODULE_AUTHOR > in the driver code. > > Best Regards, > > Boris > > Boris Brezillon (2): > crypto: add new driver for Marvell CESA > crypto: marvell/CESA: update DT bindings documentation > > .../devicetree/bindings/crypto/mv_cesa.txt | 50 +- > drivers/crypto/Kconfig |2 + > drivers/crypto/Makefile|2 +- > drivers/crypto/marvell/Makefile|1 + > drivers/crypto/marvell/cesa.c | 539 > drivers/crypto/marvell/cesa.h | 802 > drivers/crypto/marvell/cipher.c| 761 +++ > drivers/crypto/marvell/hash.c | 1349 > > drivers/crypto/marvell/tdma.c | 223 > drivers/crypto/mv_cesa.c | 1193 - > drivers/crypto/mv_cesa.h | 150 --- > 11 files changed, 3716 insertions(+), 1356 deletions(-) > create mode 100644 drivers/crypto/marvell/Makefile > create mode 100644 drivers/crypto/marvell/cesa.c > create mode 100644 drivers/crypto/marvell/cesa.h > create mode 100644 drivers/crypto/marvell/cipher.c > create mode 100644 drivers/crypto/marvell/hash.c > create mode 100644 drivers/crypto/marvell/tdma.c > delete mode 100644 drivers/crypto/mv_cesa.c > delete mode 100644 drivers/crypto/mv_cesa.h > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Herbert, David, Any comment on the crypto driver implementation ? I've had several reviews focused on: 1/ splitting the patch series into smaller subsets 2/ allowing for smoother transition from the old driver to the new one I'll address (or have addressed) all of these comments, but I'd like to have your opinion on the crypto driver itself. In particular, I'd like to discuss the threaded-irq approach taken in this driver (other drivers are using tasklets). The main reason behind this choice is the fact that crypto engines are quite fast, and I'm worried about the CPU contention that might happen in case of fully loaded crypto engines (the CPU might spend most of its time in interrupt context until the crypto queue is emptied). Using threaded-irq allows preemption of the crypto completion operation, thus authorizing another thread (with higher priority) to be scheduled. ITOH, the tasklet approach provide slightly performances (I don't recall the exact numbers, but Arnaud did some tests). On Thu, 9 Apr 2015 16:58:41 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hello, This is an attempt to replace the mv_cesa driver by a new one to address some limitations of the existing driver. From a performance and CPU load point of view the most important limitation is the lack of DMA support, thus preventing us from chaining crypto operations. I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. Here are the main features brought by this new driver: - support for armada SoCs (up to 38x) while keeping support for older ones (Orion and Kirkwood) - DMA mode to offload the CPU in case of intensive crypto usage - new algorithms: SHA256, DES and 3DES I'd like to thank Arnaud, who has carefully reviewed several iterations of this driver, helped me improved my implementation, provided support for several crypto algorithms, provided support for armada-370 and tested the driver on different platforms, hence the SoB and dual MODULE_AUTHOR in the driver code. Best Regards, Boris Boris Brezillon (2): crypto: add new driver for Marvell CESA crypto: marvell/CESA: update DT bindings documentation .../devicetree/bindings/crypto/mv_cesa.txt | 50 +- drivers/crypto/Kconfig |2 + drivers/crypto/Makefile|2 +- drivers/crypto/marvell/Makefile|1 + drivers/crypto/marvell/cesa.c | 539 drivers/crypto/marvell/cesa.h | 802 drivers/crypto/marvell/cipher.c| 761 +++ drivers/crypto/marvell/hash.c | 1349 drivers/crypto/marvell/tdma.c | 223 drivers/crypto/mv_cesa.c | 1193 - drivers/crypto/mv_cesa.h | 150 --- 11 files changed, 3716 insertions(+), 1356 deletions(-) create mode 100644 drivers/crypto/marvell/Makefile create mode 100644 drivers/crypto/marvell/cesa.c create mode 100644 drivers/crypto/marvell/cesa.h create mode 100644 drivers/crypto/marvell/cipher.c create mode 100644 drivers/crypto/marvell/hash.c create mode 100644 drivers/crypto/marvell/tdma.c delete mode 100644 drivers/crypto/mv_cesa.c delete mode 100644 drivers/crypto/mv_cesa.h -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On 17/04/2015 17:49, Maxime Ripard wrote: > On Fri, Apr 17, 2015 at 05:01:55PM +0200, Gregory CLEMENT wrote: >> On 17/04/2015 16:50, Maxime Ripard wrote: >>> On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: > On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: >> Hi Gregory, >> >> On Fri, 17 Apr 2015 15:01:01 +0200 >> Gregory CLEMENT wrote: >> >>> Hi Boris, >>> >>> On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon wrote: > Hi Jason, > > On Mon, 13 Apr 2015 20:11:46 + > Jason Cooper wrote: > >>> I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. >>> >>> Jason, what do you think of the approach above? >> >> I say keep it simple. We shouldn't use the DT changes to trigger one >> vice the other. We need to be able to build both, but only load one >> at >> a time. If that's anything other than simple to do, then we make it >> a >> Kconfig binary choice and move on. > > Actually I was planning to handle it with a Kconfig dependency rule > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > on !NEW_DRIVER). > I don't know how to make it a runtime check without adding new > compatible strings for the kirkwood, dove and orion platforms, and I'm > sure sure this is a good idea. ^ not > Do you have any ideas ? >>> >>> You use devm_ioremap_resource() in the new driver, so if the old one >>> is already loaded the memory region will be already hold and the new >>> driver will simply fail during the probe. So for this part it is OK. >> >> I like the idea :-). > > Not really, how do you know which device is going to be probed? For > that matter, it's pretty much random, and you have no control over it. > > Why not just have a choice option, and select which one you want to > enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. >>> >>> Well, actually, you don't even know if it's going to be a module. You >>> might very well have both drivers compiled statically in the kernel >>> image, and this is where the trouble begins. >> >> No it won't be possible, Boris already speak about this issue (see below): >> "Actually I was planning to handle it with a Kconfig dependency rule >> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends >> on !NEW_DRIVER)." > > Which is a circular dependency and won't work. Indeed. Boris what about using choice/endchoice ? Thanks, Gregory > So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. >>> >>> Of course, but this is already there, and doesn't really address the >>> same issue. >> >> This was the only issue remaining, (see below again): >> "I don't know how to make it a runtime check ". And my last emails >> was bout it. > > Ok, my bad then :) > > Maxime > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, Apr 17, 2015 at 05:01:55PM +0200, Gregory CLEMENT wrote: > On 17/04/2015 16:50, Maxime Ripard wrote: > > On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: > >> Hi Maxime, > >> > >> On 17/04/2015 16:32, Maxime Ripard wrote: > >>> On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: > Hi Gregory, > > On Fri, 17 Apr 2015 15:01:01 +0200 > Gregory CLEMENT wrote: > > > Hi Boris, > > > > On 17/04/2015 10:39, Boris Brezillon wrote: > >> On Fri, 17 Apr 2015 10:33:56 +0200 > >> Boris Brezillon wrote: > >> > >>> Hi Jason, > >>> > >>> On Mon, 13 Apr 2015 20:11:46 + > >>> Jason Cooper wrote: > >>> > > > >> I'd appreciate if we'd look into it. I understand from on-list and > >> off-list discussion that the rewrite was unavoidable. So I'm > >> willing to > >> concede that. Giving people time to migrate from old to new while > >> still > >> being able to update for other security fixes seems reasonable. > > > > Jason, what do you think of the approach above? > > I say keep it simple. We shouldn't use the DT changes to trigger one > vice the other. We need to be able to build both, but only load one > at > a time. If that's anything other than simple to do, then we make it > a > Kconfig binary choice and move on. > >>> > >>> Actually I was planning to handle it with a Kconfig dependency rule > >>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > >>> on !NEW_DRIVER). > >>> I don't know how to make it a runtime check without adding new > >>> compatible strings for the kirkwood, dove and orion platforms, and I'm > >>> sure sure this is a good idea. > >> ^ not > >> > >>> Do you have any ideas ? > > > > You use devm_ioremap_resource() in the new driver, so if the old one > > is already loaded the memory region will be already hold and the new > > driver will simply fail during the probe. So for this part it is OK. > > I like the idea :-). > >>> > >>> Not really, how do you know which device is going to be probed? For > >>> that matter, it's pretty much random, and you have no control over it. > >>> > >>> Why not just have a choice option, and select which one you want to > >>> enable? > >> > >> Because you can't prevent an user to build a module, then modifying the > >> configuration and building the other module. > > > > Well, actually, you don't even know if it's going to be a module. You > > might very well have both drivers compiled statically in the kernel > > image, and this is where the trouble begins. > > No it won't be possible, Boris already speak about this issue (see below): > "Actually I was planning to handle it with a Kconfig dependency rule > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > on !NEW_DRIVER)." Which is a circular dependency and won't work. > >> So even if there is a choice at build time, and I think that it is > >> something expected for the v2, we still need preventing having the > >> both drivers trying accessing the same hardware in the same time. > > > > Of course, but this is already there, and doesn't really address the > > same issue. > > This was the only issue remaining, (see below again): > "I don't know how to make it a runtime check ". And my last emails > was bout it. Ok, my bad then :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On 17/04/2015 16:50, Maxime Ripard wrote: > On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: >> Hi Maxime, >> >> On 17/04/2015 16:32, Maxime Ripard wrote: >>> On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT wrote: > Hi Boris, > > On 17/04/2015 10:39, Boris Brezillon wrote: >> On Fri, 17 Apr 2015 10:33:56 +0200 >> Boris Brezillon wrote: >> >>> Hi Jason, >>> >>> On Mon, 13 Apr 2015 20:11:46 + >>> Jason Cooper wrote: >>> > >> I'd appreciate if we'd look into it. I understand from on-list and >> off-list discussion that the rewrite was unavoidable. So I'm >> willing to >> concede that. Giving people time to migrate from old to new while >> still >> being able to update for other security fixes seems reasonable. > > Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. >>> >>> Actually I was planning to handle it with a Kconfig dependency rule >>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends >>> on !NEW_DRIVER). >>> I don't know how to make it a runtime check without adding new >>> compatible strings for the kirkwood, dove and orion platforms, and I'm >>> sure sure this is a good idea. >> ^ not >> >>> Do you have any ideas ? > > You use devm_ioremap_resource() in the new driver, so if the old one > is already loaded the memory region will be already hold and the new > driver will simply fail during the probe. So for this part it is OK. I like the idea :-). >>> >>> Not really, how do you know which device is going to be probed? For >>> that matter, it's pretty much random, and you have no control over it. >>> >>> Why not just have a choice option, and select which one you want to >>> enable? >> >> Because you can't prevent an user to build a module, then modifying the >> configuration and building the other module. > > Well, actually, you don't even know if it's going to be a module. You > might very well have both drivers compiled statically in the kernel > image, and this is where the trouble begins. No it won't be possible, Boris already speak about this issue (see below): "Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER)." > >> So even if there is a choice at build time, and I think that it is >> something expected for the v2, we still need preventing having the >> both drivers trying accessing the same hardware in the same time. > > Of course, but this is already there, and doesn't really address the > same issue. This was the only issue remaining, (see below again): "I don't know how to make it a runtime check ". And my last emails was bout it. Gregory > > Maxime > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: > Hi Maxime, > > On 17/04/2015 16:32, Maxime Ripard wrote: > > On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: > >> Hi Gregory, > >> > >> On Fri, 17 Apr 2015 15:01:01 +0200 > >> Gregory CLEMENT wrote: > >> > >>> Hi Boris, > >>> > >>> On 17/04/2015 10:39, Boris Brezillon wrote: > On Fri, 17 Apr 2015 10:33:56 +0200 > Boris Brezillon wrote: > > > Hi Jason, > > > > On Mon, 13 Apr 2015 20:11:46 + > > Jason Cooper wrote: > > > >>> > I'd appreciate if we'd look into it. I understand from on-list and > off-list discussion that the rewrite was unavoidable. So I'm > willing to > concede that. Giving people time to migrate from old to new while > still > being able to update for other security fixes seems reasonable. > >>> > >>> Jason, what do you think of the approach above? > >> > >> I say keep it simple. We shouldn't use the DT changes to trigger one > >> vice the other. We need to be able to build both, but only load one at > >> a time. If that's anything other than simple to do, then we make it a > >> Kconfig binary choice and move on. > > > > Actually I was planning to handle it with a Kconfig dependency rule > > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > > on !NEW_DRIVER). > > I don't know how to make it a runtime check without adding new > > compatible strings for the kirkwood, dove and orion platforms, and I'm > > sure sure this is a good idea. > ^ not > > > Do you have any ideas ? > >>> > >>> You use devm_ioremap_resource() in the new driver, so if the old one > >>> is already loaded the memory region will be already hold and the new > >>> driver will simply fail during the probe. So for this part it is OK. > >> > >> I like the idea :-). > > > > Not really, how do you know which device is going to be probed? For > > that matter, it's pretty much random, and you have no control over it. > > > > Why not just have a choice option, and select which one you want to > > enable? > > Because you can't prevent an user to build a module, then modifying the > configuration and building the other module. Well, actually, you don't even know if it's going to be a module. You might very well have both drivers compiled statically in the kernel image, and this is where the trouble begins. > So even if there is a choice at build time, and I think that it is > something expected for the v2, we still need preventing having the > both drivers trying accessing the same hardware in the same time. Of course, but this is already there, and doesn't really address the same issue. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: > On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: >> Hi Gregory, >> >> On Fri, 17 Apr 2015 15:01:01 +0200 >> Gregory CLEMENT wrote: >> >>> Hi Boris, >>> >>> On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon wrote: > Hi Jason, > > On Mon, 13 Apr 2015 20:11:46 + > Jason Cooper wrote: > >>> I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. >>> >>> Jason, what do you think of the approach above? >> >> I say keep it simple. We shouldn't use the DT changes to trigger one >> vice the other. We need to be able to build both, but only load one at >> a time. If that's anything other than simple to do, then we make it a >> Kconfig binary choice and move on. > > Actually I was planning to handle it with a Kconfig dependency rule > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > on !NEW_DRIVER). > I don't know how to make it a runtime check without adding new > compatible strings for the kirkwood, dove and orion platforms, and I'm > sure sure this is a good idea. ^ not > Do you have any ideas ? >>> >>> You use devm_ioremap_resource() in the new driver, so if the old one >>> is already loaded the memory region will be already hold and the new >>> driver will simply fail during the probe. So for this part it is OK. >> >> I like the idea :-). > > Not really, how do you know which device is going to be probed? For > that matter, it's pretty much random, and you have no control over it. > > Why not just have a choice option, and select which one you want to > enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. Thanks, Gregory > > Maxime > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: > Hi Gregory, > > On Fri, 17 Apr 2015 15:01:01 +0200 > Gregory CLEMENT wrote: > > > Hi Boris, > > > > On 17/04/2015 10:39, Boris Brezillon wrote: > > > On Fri, 17 Apr 2015 10:33:56 +0200 > > > Boris Brezillon wrote: > > > > > >> Hi Jason, > > >> > > >> On Mon, 13 Apr 2015 20:11:46 + > > >> Jason Cooper wrote: > > >> > > > > > I'd appreciate if we'd look into it. I understand from on-list and > > > off-list discussion that the rewrite was unavoidable. So I'm willing > > > to > > > concede that. Giving people time to migrate from old to new while > > > still > > > being able to update for other security fixes seems reasonable. > > > > Jason, what do you think of the approach above? > > >>> > > >>> I say keep it simple. We shouldn't use the DT changes to trigger one > > >>> vice the other. We need to be able to build both, but only load one at > > >>> a time. If that's anything other than simple to do, then we make it a > > >>> Kconfig binary choice and move on. > > >> > > >> Actually I was planning to handle it with a Kconfig dependency rule > > >> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > > >> on !NEW_DRIVER). > > >> I don't know how to make it a runtime check without adding new > > >> compatible strings for the kirkwood, dove and orion platforms, and I'm > > >> sure sure this is a good idea. > > > ^ not > > > > > >> Do you have any ideas ? > > > > You use devm_ioremap_resource() in the new driver, so if the old one > > is already loaded the memory region will be already hold and the new > > driver will simply fail during the probe. So for this part it is OK. > > I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT wrote: > Hi Boris, > > On 17/04/2015 10:39, Boris Brezillon wrote: > > On Fri, 17 Apr 2015 10:33:56 +0200 > > Boris Brezillon wrote: > > > >> Hi Jason, > >> > >> On Mon, 13 Apr 2015 20:11:46 + > >> Jason Cooper wrote: > >> > > > I'd appreciate if we'd look into it. I understand from on-list and > > off-list discussion that the rewrite was unavoidable. So I'm willing to > > concede that. Giving people time to migrate from old to new while still > > being able to update for other security fixes seems reasonable. > > Jason, what do you think of the approach above? > >>> > >>> I say keep it simple. We shouldn't use the DT changes to trigger one > >>> vice the other. We need to be able to build both, but only load one at > >>> a time. If that's anything other than simple to do, then we make it a > >>> Kconfig binary choice and move on. > >> > >> Actually I was planning to handle it with a Kconfig dependency rule > >> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > >> on !NEW_DRIVER). > >> I don't know how to make it a runtime check without adding new > >> compatible strings for the kirkwood, dove and orion platforms, and I'm > >> sure sure this is a good idea. > > ^ not > > > >> Do you have any ideas ? > > You use devm_ioremap_resource() in the new driver, so if the old one > is already loaded the memory region will be already hold and the new > driver will simply fail during the probe. So for this part it is OK. I like the idea :-). > > However, the old driver doesn't try to reserve the region, it directly > uses an ioremap(). So if the new driver is loaded first, then the old > one will manage to be loaded too. I think that just adding a > request_region()/release_region() (or converting the ioremap in a > devm_ioremap_resource() in the old driver would be enough. Absolutely. Unless someone is opposed to this solution I think I'll choose this solution. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: > On Fri, 17 Apr 2015 10:33:56 +0200 > Boris Brezillon wrote: > >> Hi Jason, >> >> On Mon, 13 Apr 2015 20:11:46 + >> Jason Cooper wrote: >> > I'd appreciate if we'd look into it. I understand from on-list and > off-list discussion that the rewrite was unavoidable. So I'm willing to > concede that. Giving people time to migrate from old to new while still > being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? >>> >>> I say keep it simple. We shouldn't use the DT changes to trigger one >>> vice the other. We need to be able to build both, but only load one at >>> a time. If that's anything other than simple to do, then we make it a >>> Kconfig binary choice and move on. >> >> Actually I was planning to handle it with a Kconfig dependency rule >> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends >> on !NEW_DRIVER). >> I don't know how to make it a runtime check without adding new >> compatible strings for the kirkwood, dove and orion platforms, and I'm >> sure sure this is a good idea. > ^ not > >> Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. However, the old driver doesn't try to reserve the region, it directly uses an ioremap(). So if the new driver is loaded first, then the old one will manage to be loaded too. I think that just adding a request_region()/release_region() (or converting the ioremap in a devm_ioremap_resource() in the old driver would be enough. Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Boris, On Fri, Apr 17, 2015 at 10:39:46AM +0200, Boris Brezillon wrote: > On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon > wrote: > > On Mon, 13 Apr 2015 20:11:46 + Jason Cooper > > wrote: > > > > > > > > > I'd appreciate if we'd look into it. I understand from on-list and > > > > > off-list discussion that the rewrite was unavoidable. So I'm willing > > > > > to > > > > > concede that. Giving people time to migrate from old to new while > > > > > still > > > > > being able to update for other security fixes seems reasonable. > > > > > > > > Jason, what do you think of the approach above? > > > > > > I say keep it simple. We shouldn't use the DT changes to trigger one > > > vice the other. We need to be able to build both, but only load one at > > > a time. If that's anything other than simple to do, then we make it a > > > Kconfig binary choice and move on. > > > > Actually I was planning to handle it with a Kconfig dependency rule > > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > > on !NEW_DRIVER). > > I don't know how to make it a runtime check without adding new > > compatible strings for the kirkwood, dove and orion platforms, and I'm > > sure sure this is a good idea. > ^ not > > > Do you have any ideas ? I'm kinda wrapped up with dayjob stuff atm... But I'd look at the wireless drivers. eg b43, b43legacy, brcm80211. There are devices they overlap for. So, they need to deconflict in some way. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon wrote: > Hi Jason, > > On Mon, 13 Apr 2015 20:11:46 + > Jason Cooper wrote: > > > > > > > > I'd appreciate if we'd look into it. I understand from on-list and > > > > off-list discussion that the rewrite was unavoidable. So I'm willing to > > > > concede that. Giving people time to migrate from old to new while still > > > > being able to update for other security fixes seems reasonable. > > > > > > Jason, what do you think of the approach above? > > > > I say keep it simple. We shouldn't use the DT changes to trigger one > > vice the other. We need to be able to build both, but only load one at > > a time. If that's anything other than simple to do, then we make it a > > Kconfig binary choice and move on. > > Actually I was planning to handle it with a Kconfig dependency rule > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends > on !NEW_DRIVER). > I don't know how to make it a runtime check without adding new > compatible strings for the kirkwood, dove and orion platforms, and I'm > sure sure this is a good idea. ^ not > Do you have any ideas ? > > Regards, > > Boris > > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper wrote: > > > > > I'd appreciate if we'd look into it. I understand from on-list and > > > off-list discussion that the rewrite was unavoidable. So I'm willing to > > > concede that. Giving people time to migrate from old to new while still > > > being able to update for other security fixes seems reasonable. > > > > Jason, what do you think of the approach above? > > I say keep it simple. We shouldn't use the DT changes to trigger one > vice the other. We need to be able to build both, but only load one at > a time. If that's anything other than simple to do, then we make it a > Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. Do you have any ideas ? Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. Do you have any ideas ? Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. However, the old driver doesn't try to reserve the region, it directly uses an ioremap(). So if the new driver is loaded first, then the old one will manage to be loaded too. I think that just adding a request_region()/release_region() (or converting the ioremap in a devm_ioremap_resource() in the old driver would be enough. Gregory -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Boris, On Fri, Apr 17, 2015 at 10:39:46AM +0200, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? I'm kinda wrapped up with dayjob stuff atm... But I'd look at the wireless drivers. eg b43, b43legacy, brcm80211. There are devices they overlap for. So, they need to deconflict in some way. thx, Jason. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. Thanks, Gregory Maxime -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). However, the old driver doesn't try to reserve the region, it directly uses an ioremap(). So if the new driver is loaded first, then the old one will manage to be loaded too. I think that just adding a request_region()/release_region() (or converting the ioremap in a devm_ioremap_resource() in the old driver would be enough. Absolutely. Unless someone is opposed to this solution I think I'll choose this solution. Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. Well, actually, you don't even know if it's going to be a module. You might very well have both drivers compiled statically in the kernel image, and this is where the trouble begins. So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. Of course, but this is already there, and doesn't really address the same issue. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On 17/04/2015 16:50, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. Well, actually, you don't even know if it's going to be a module. You might very well have both drivers compiled statically in the kernel image, and this is where the trouble begins. No it won't be possible, Boris already speak about this issue (see below): Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. Of course, but this is already there, and doesn't really address the same issue. This was the only issue remaining, (see below again): I don't know how to make it a runtime check . And my last emails was bout it. Gregory Maxime -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On 17/04/2015 17:49, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 05:01:55PM +0200, Gregory CLEMENT wrote: On 17/04/2015 16:50, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. Well, actually, you don't even know if it's going to be a module. You might very well have both drivers compiled statically in the kernel image, and this is where the trouble begins. No it won't be possible, Boris already speak about this issue (see below): Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). Which is a circular dependency and won't work. Indeed. Boris what about using choice/endchoice ? Thanks, Gregory So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. Of course, but this is already there, and doesn't really address the same issue. This was the only issue remaining, (see below again): I don't know how to make it a runtime check . And my last emails was bout it. Ok, my bad then :) Maxime -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Fri, Apr 17, 2015 at 05:01:55PM +0200, Gregory CLEMENT wrote: On 17/04/2015 16:50, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote: Hi Maxime, On 17/04/2015 16:32, Maxime Ripard wrote: On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote: Hi Gregory, On Fri, 17 Apr 2015 15:01:01 +0200 Gregory CLEMENT gregory.clem...@free-electrons.com wrote: Hi Boris, On 17/04/2015 10:39, Boris Brezillon wrote: On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon boris.brezil...@free-electrons.com wrote: Hi Jason, On Mon, 13 Apr 2015 20:11:46 + Jason Cooper ja...@lakedaemon.net wrote: I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). I don't know how to make it a runtime check without adding new compatible strings for the kirkwood, dove and orion platforms, and I'm sure sure this is a good idea. ^ not Do you have any ideas ? You use devm_ioremap_resource() in the new driver, so if the old one is already loaded the memory region will be already hold and the new driver will simply fail during the probe. So for this part it is OK. I like the idea :-). Not really, how do you know which device is going to be probed? For that matter, it's pretty much random, and you have no control over it. Why not just have a choice option, and select which one you want to enable? Because you can't prevent an user to build a module, then modifying the configuration and building the other module. Well, actually, you don't even know if it's going to be a module. You might very well have both drivers compiled statically in the kernel image, and this is where the trouble begins. No it won't be possible, Boris already speak about this issue (see below): Actually I was planning to handle it with a Kconfig dependency rule (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends on !NEW_DRIVER). Which is a circular dependency and won't work. So even if there is a choice at build time, and I think that it is something expected for the v2, we still need preventing having the both drivers trying accessing the same hardware in the same time. Of course, but this is already there, and doesn't really address the same issue. This was the only issue remaining, (see below again): I don't know how to make it a runtime check . And my last emails was bout it. Ok, my bad then :) Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Arnaud, On Mon, Apr 13, 2015 at 06:06:49PM +0200, Arnaud Ebalard wrote: > Jason Cooper writes: ... > >> >> I really tried to adapt the existing driver to add the missing > >> >> features (especially the support for TDMA), but all my attempts > >> >> ended up introducing hackish code (not even talking about the > >> >> performance penalty of this approach). > >> > > >> > Ok, fair enough. It would be helpful if this account of attempting to > >> > reconcile the old driver made it into the commit message. This puts us > >> > in "perfect is the enemy of getting it done" territory. > >> > > >> >> I have another solution though: keep the existing driver for old > >> >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern > >> >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver > >> >> won't have to audit the new code. > >> > > >> > A fair proposal, but I'll freely admit the number of people actually > >> > auditing > >> > their code paths is orders of magnitude smaller than the number of users > >> > of the driver. > >> > > >> > There's such a large population of compatible legacy SoCs in the wild, > >> > adding an artificial boundary doesn't make sense. Especially since > >> > we're talking about features everyone would want to use. > >> > > >> > Perhaps we should keep both around, and deprecate the legacy driver over > >> > 3 to 4 cycles? > >> > >> But I guess that some users will want to use the new driver on the "old" > >> marvell > >> SoCs (especially kirkwood and dove). > > > > Yes, despite my arguments, I'm one of those people. :-P > > > >> If we go to this path, then the best solution would be to still update > >> all the the dts, and modifying the old driver to be able to use the > >> new binding: for my point of view the only adaptation should be > >> related to the SRAM. It will be also needed to find a way to be able > >> to load only one driver at a time: either the old or the new, but not > >> both. > > The approach Boris proposed above seems to make everyone happy: > > 1) Keep the old driver for old marvells SoCs (kirkwood, dove and orion) > 2) Introduce the new driver for those that are not supported by the old > driver, i.e. armada (370, XP, 375, 38x) > > AFAICT, this can easily be done (based on compatible strings) and it > will let everyone the time to audit the new driver. Current users will > not be taken by surprise. At some point, when everyone is confident w/ > the new driver, we can then switch to that one for all SoCs so that > old platform get more performance. > > Additionnally, for those who want to get the feature of the new driver > for their old SoC right now, we *could* add a simple kernel config option > for the new driver to use it for the old SoC too (that one disabling the > old one). > > > > I'd appreciate if we'd look into it. I understand from on-list and > > off-list discussion that the rewrite was unavoidable. So I'm willing to > > concede that. Giving people time to migrate from old to new while still > > being able to update for other security fixes seems reasonable. > > Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Jason, Jason Cooper writes: > It's not about the crypto, it's about trust. imho, one of the most > important security advances in the past 20 years is the default use of > git (or other SCMs) by open source projects. Now, no one is forced to > trust the authors and maintainers tarball dumps. Regular code audits > and security updates are *much* more feasible because you can audit > small changes. It can even be automated to a large extent. > > All this means the user has a choice: they can trust the authors and > maintainers, or they can trust their own audits. Since updates are an > essential part of a security posture, small commits facilitate > maintaining the 'trust in audits'. > > It's not about "Should you trust free-electrons?" Or, "Should you trust > Jason / Herbert / Linus?" It's about "Should you have to trust any of > them?" It's ok, you can call our driver fat. It is ;-) More seriously, I tend to agree w/ what you write above. >> >> I really tried to adapt the existing driver to add the missing >> >> features (especially the support for TDMA), but all my attempts >> >> ended up introducing hackish code (not even talking about the >> >> performance penalty of this approach). >> > >> > Ok, fair enough. It would be helpful if this account of attempting to >> > reconcile the old driver made it into the commit message. This puts us >> > in "perfect is the enemy of getting it done" territory. >> > >> >> I have another solution though: keep the existing driver for old >> >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern >> >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver >> >> won't have to audit the new code. >> > >> > A fair proposal, but I'll freely admit the number of people actually >> > auditing >> > their code paths is orders of magnitude smaller than the number of users >> > of the driver. >> > >> > There's such a large population of compatible legacy SoCs in the wild, >> > adding an artificial boundary doesn't make sense. Especially since >> > we're talking about features everyone would want to use. >> > >> > Perhaps we should keep both around, and deprecate the legacy driver over >> > 3 to 4 cycles? >> >> But I guess that some users will want to use the new driver on the "old" >> marvell >> SoCs (especially kirkwood and dove). > > Yes, despite my arguments, I'm one of those people. :-P > >> If we go to this path, then the best solution would be to still update >> all the the dts, and modifying the old driver to be able to use the >> new binding: for my point of view the only adaptation should be >> related to the SRAM. It will be also needed to find a way to be able >> to load only one driver at a time: either the old or the new, but not >> both. The approach Boris proposed above seems to make everyone happy: 1) Keep the old driver for old marvells SoCs (kirkwood, dove and orion) 2) Introduce the new driver for those that are not supported by the old driver, i.e. armada (370, XP, 375, 38x) AFAICT, this can easily be done (based on compatible strings) and it will let everyone the time to audit the new driver. Current users will not be taken by surprise. At some point, when everyone is confident w/ the new driver, we can then switch to that one for all SoCs so that old platform get more performance. Additionnally, for those who want to get the feature of the new driver for their old SoC right now, we *could* add a simple kernel config option for the new driver to use it for the old SoC too (that one disabling the old one). > I'd appreciate if we'd look into it. I understand from on-list and > off-list discussion that the rewrite was unavoidable. So I'm willing to > concede that. Giving people time to migrate from old to new while still > being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? Cheers, a+ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Gregory, On Mon, Apr 13, 2015 at 11:39:16AM +0200, Gregory CLEMENT wrote: > Hi Jason, Boris, > > On 11/04/2015 00:30, Jason Cooper wrote: > > Hey Boris, > > > > On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote: > >> On Fri, 10 Apr 2015 13:50:56 + Jason Cooper > >> wrote: > >>> On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: > I know we usually try to adapt existing drivers instead of replacing them > by new ones, but after trying to refactor the mv_cesa driver I realized > it > would take longer than writing an new one from scratch. > >>> > >>> I'm sorry, but this makes me *very* uncomfortable. Any organization > >>> worth it's salt will do a very careful audit of code touching > >>> cryptographic material and sensitive (decrypted) data. From that point > >>> on, small audits of the changes to the code allow the organization to > >>> build a comfort level with kernel updates. iow, following the git > >>> history of the driver. > >>> > >>> By apply this series, we are basically forcing those organizations to > >>> either a) stop updating, or b) expend significant resources to do > >>> another full audit. > >>> > >>> In short, this series breaks the audit chain for the mv_cesa driver. > >>> > >>> Maybe I'm the only person with this level of paranoia. If so, I'm sure > >>> others will override me. > >>> > >>> From my POV, it looks like the *only* reason we've chosen this route is > >>> developer convenience. I don't think that's sufficient reason to break > >>> the change history of a driver handling sensitive data. > >> > >> Well, I understand you concern, but if you read carefully the old and > >> new drivers, you'll notice that they are completely different (the only > >> thing I kept are the macro definitions). > > > > Yes, that's the worrying part for me. ;-) > > I understand the logic behind your concern, but I wonder if it is really > an issue. My knowledge ans my background around crypto is very limited, > so I really would like the opinion of other people on the subject. It's not about the crypto, it's about trust. imho, one of the most important security advances in the past 20 years is the default use of git (or other SCMs) by open source projects. Now, no one is forced to trust the authors and maintainers tarball dumps. Regular code audits and security updates are *much* more feasible because you can audit small changes. It can even be automated to a large extent. All this means the user has a choice: they can trust the authors and maintainers, or they can trust their own audits. Since updates are an essential part of a security posture, small commits facilitate maintaining the 'trust in audits'. It's not about "Should you trust free-electrons?" Or, "Should you trust Jason / Herbert / Linus?" It's about "Should you have to trust any of them?" > >> I really tried to adapt the existing driver to add the missing > >> features (especially the support for TDMA), but all my attempts > >> ended up introducing hackish code (not even talking about the > >> performance penalty of this approach). > > > > Ok, fair enough. It would be helpful if this account of attempting to > > reconcile the old driver made it into the commit message. This puts us > > in "perfect is the enemy of getting it done" territory. > > > >> I have another solution though: keep the existing driver for old > >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern > >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver > >> won't have to audit the new code. > > > > A fair proposal, but I'll freely admit the number of people actually > > auditing > > their code paths is orders of magnitude smaller than the number of users > > of the driver. > > > > There's such a large population of compatible legacy SoCs in the wild, > > adding an artificial boundary doesn't make sense. Especially since > > we're talking about features everyone would want to use. > > > > Perhaps we should keep both around, and deprecate the legacy driver over > > 3 to 4 cycles? > > But I guess that some users will want to use the new driver on the "old" > marvell > SoCs (especially kirkwood and dove). Yes, despite my arguments, I'm one of those people. :-P > If we go to this path, then the best solution would be to still update > all the the dts, and modifying the old driver to be able to use the > new binding: for my point of view the only adaptation should be > related to the SRAM. It will be also needed to find a way to be able > to load only one driver at a time: either the old or the new, but not > both. We can look at how the wireless drivers handle this. They often have to choose between multiple drivers (foss, proprietary, ndis-something, etc) for a given card. Not much different here. > However I still wonder if it worth the effort. I'd appreciate if we'd look into it. I understand from on-list and off-list discussion
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Jason, Boris, On 11/04/2015 00:30, Jason Cooper wrote: > Hey Boris, > > On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote: >> On Fri, 10 Apr 2015 13:50:56 + Jason Cooper wrote: >>> On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. >>> >>> I'm sorry, but this makes me *very* uncomfortable. Any organization >>> worth it's salt will do a very careful audit of code touching >>> cryptographic material and sensitive (decrypted) data. From that point >>> on, small audits of the changes to the code allow the organization to >>> build a comfort level with kernel updates. iow, following the git >>> history of the driver. >>> >>> By apply this series, we are basically forcing those organizations to >>> either a) stop updating, or b) expend significant resources to do >>> another full audit. >>> >>> In short, this series breaks the audit chain for the mv_cesa driver. >>> >>> Maybe I'm the only person with this level of paranoia. If so, I'm sure >>> others will override me. >>> >>> From my POV, it looks like the *only* reason we've chosen this route is >>> developer convenience. I don't think that's sufficient reason to break >>> the change history of a driver handling sensitive data. >> >> Well, I understand you concern, but if you read carefully the old and >> new drivers, you'll notice that they are completely different (the only >> thing I kept are the macro definitions). > > Yes, that's the worrying part for me. ;-) I understand the logic behind your concern, but I wonder if it is really an issue. My knowledge ans my background around crypto is very limited, so I really would like the opinion of other people on the subject. > >> I really tried to adapt the existing driver to add the missing >> features (especially the support for TDMA), but all my attempts >> ended up introducing hackish code (not even talking about the >> performance penalty of this approach). > > Ok, fair enough. It would be helpful if this account of attempting to > reconcile the old driver made it into the commit message. This puts us > in "perfect is the enemy of getting it done" territory. > >> I have another solution though: keep the existing driver for old >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver >> won't have to audit the new code. > > A fair proposal, but I'll freely admit the number of people actually auditing > their code paths is orders of magnitude smaller than the number of users > of the driver. > > There's such a large population of compatible legacy SoCs in the wild, > adding an artificial boundary doesn't make sense. Especially since > we're talking about features everyone would want to use. > > Perhaps we should keep both around, and deprecate the legacy driver over > 3 to 4 cycles? But I guess that some users will want to use the new driver on the "old" marvell SoCs (especially kirkwood and dove). If we go to this path, then the best solution would be to still update all the the dts, and modifying the old driver to be able to use the new binding: for my point of view the only adaptation should be related to the SRAM. It will be also needed to find a way to be able to load only one driver at a time: either the old or the new, but not both. However I still wonder if it worth the effort. Thanks, Gregory > > thx, > > Jason. > -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Arnaud, On Mon, Apr 13, 2015 at 06:06:49PM +0200, Arnaud Ebalard wrote: Jason Cooper ja...@lakedaemon.net writes: ... I really tried to adapt the existing driver to add the missing features (especially the support for TDMA), but all my attempts ended up introducing hackish code (not even talking about the performance penalty of this approach). Ok, fair enough. It would be helpful if this account of attempting to reconcile the old driver made it into the commit message. This puts us in perfect is the enemy of getting it done territory. I have another solution though: keep the existing driver for old marvell SoCs (orion, kirkwood and dove), and add a new one for modern SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver won't have to audit the new code. A fair proposal, but I'll freely admit the number of people actually auditing their code paths is orders of magnitude smaller than the number of users of the driver. There's such a large population of compatible legacy SoCs in the wild, adding an artificial boundary doesn't make sense. Especially since we're talking about features everyone would want to use. Perhaps we should keep both around, and deprecate the legacy driver over 3 to 4 cycles? But I guess that some users will want to use the new driver on the old marvell SoCs (especially kirkwood and dove). Yes, despite my arguments, I'm one of those people. :-P If we go to this path, then the best solution would be to still update all the the dts, and modifying the old driver to be able to use the new binding: for my point of view the only adaptation should be related to the SRAM. It will be also needed to find a way to be able to load only one driver at a time: either the old or the new, but not both. The approach Boris proposed above seems to make everyone happy: 1) Keep the old driver for old marvells SoCs (kirkwood, dove and orion) 2) Introduce the new driver for those that are not supported by the old driver, i.e. armada (370, XP, 375, 38x) AFAICT, this can easily be done (based on compatible strings) and it will let everyone the time to audit the new driver. Current users will not be taken by surprise. At some point, when everyone is confident w/ the new driver, we can then switch to that one for all SoCs so that old platform get more performance. Additionnally, for those who want to get the feature of the new driver for their old SoC right now, we *could* add a simple kernel config option for the new driver to use it for the old SoC too (that one disabling the old one). I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? I say keep it simple. We shouldn't use the DT changes to trigger one vice the other. We need to be able to build both, but only load one at a time. If that's anything other than simple to do, then we make it a Kconfig binary choice and move on. thx, Jason. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Jason, Boris, On 11/04/2015 00:30, Jason Cooper wrote: Hey Boris, On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote: On Fri, 10 Apr 2015 13:50:56 + Jason Cooper ja...@lakedaemon.net wrote: On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. I'm sorry, but this makes me *very* uncomfortable. Any organization worth it's salt will do a very careful audit of code touching cryptographic material and sensitive (decrypted) data. From that point on, small audits of the changes to the code allow the organization to build a comfort level with kernel updates. iow, following the git history of the driver. By apply this series, we are basically forcing those organizations to either a) stop updating, or b) expend significant resources to do another full audit. In short, this series breaks the audit chain for the mv_cesa driver. Maybe I'm the only person with this level of paranoia. If so, I'm sure others will override me. From my POV, it looks like the *only* reason we've chosen this route is developer convenience. I don't think that's sufficient reason to break the change history of a driver handling sensitive data. Well, I understand you concern, but if you read carefully the old and new drivers, you'll notice that they are completely different (the only thing I kept are the macro definitions). Yes, that's the worrying part for me. ;-) I understand the logic behind your concern, but I wonder if it is really an issue. My knowledge ans my background around crypto is very limited, so I really would like the opinion of other people on the subject. I really tried to adapt the existing driver to add the missing features (especially the support for TDMA), but all my attempts ended up introducing hackish code (not even talking about the performance penalty of this approach). Ok, fair enough. It would be helpful if this account of attempting to reconcile the old driver made it into the commit message. This puts us in perfect is the enemy of getting it done territory. I have another solution though: keep the existing driver for old marvell SoCs (orion, kirkwood and dove), and add a new one for modern SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver won't have to audit the new code. A fair proposal, but I'll freely admit the number of people actually auditing their code paths is orders of magnitude smaller than the number of users of the driver. There's such a large population of compatible legacy SoCs in the wild, adding an artificial boundary doesn't make sense. Especially since we're talking about features everyone would want to use. Perhaps we should keep both around, and deprecate the legacy driver over 3 to 4 cycles? But I guess that some users will want to use the new driver on the old marvell SoCs (especially kirkwood and dove). If we go to this path, then the best solution would be to still update all the the dts, and modifying the old driver to be able to use the new binding: for my point of view the only adaptation should be related to the SRAM. It will be also needed to find a way to be able to load only one driver at a time: either the old or the new, but not both. However I still wonder if it worth the effort. Thanks, Gregory thx, Jason. -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Gregory, On Mon, Apr 13, 2015 at 11:39:16AM +0200, Gregory CLEMENT wrote: Hi Jason, Boris, On 11/04/2015 00:30, Jason Cooper wrote: Hey Boris, On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote: On Fri, 10 Apr 2015 13:50:56 + Jason Cooper ja...@lakedaemon.net wrote: On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. I'm sorry, but this makes me *very* uncomfortable. Any organization worth it's salt will do a very careful audit of code touching cryptographic material and sensitive (decrypted) data. From that point on, small audits of the changes to the code allow the organization to build a comfort level with kernel updates. iow, following the git history of the driver. By apply this series, we are basically forcing those organizations to either a) stop updating, or b) expend significant resources to do another full audit. In short, this series breaks the audit chain for the mv_cesa driver. Maybe I'm the only person with this level of paranoia. If so, I'm sure others will override me. From my POV, it looks like the *only* reason we've chosen this route is developer convenience. I don't think that's sufficient reason to break the change history of a driver handling sensitive data. Well, I understand you concern, but if you read carefully the old and new drivers, you'll notice that they are completely different (the only thing I kept are the macro definitions). Yes, that's the worrying part for me. ;-) I understand the logic behind your concern, but I wonder if it is really an issue. My knowledge ans my background around crypto is very limited, so I really would like the opinion of other people on the subject. It's not about the crypto, it's about trust. imho, one of the most important security advances in the past 20 years is the default use of git (or other SCMs) by open source projects. Now, no one is forced to trust the authors and maintainers tarball dumps. Regular code audits and security updates are *much* more feasible because you can audit small changes. It can even be automated to a large extent. All this means the user has a choice: they can trust the authors and maintainers, or they can trust their own audits. Since updates are an essential part of a security posture, small commits facilitate maintaining the 'trust in audits'. It's not about Should you trust free-electrons? Or, Should you trust Jason / Herbert / Linus? It's about Should you have to trust any of them? I really tried to adapt the existing driver to add the missing features (especially the support for TDMA), but all my attempts ended up introducing hackish code (not even talking about the performance penalty of this approach). Ok, fair enough. It would be helpful if this account of attempting to reconcile the old driver made it into the commit message. This puts us in perfect is the enemy of getting it done territory. I have another solution though: keep the existing driver for old marvell SoCs (orion, kirkwood and dove), and add a new one for modern SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver won't have to audit the new code. A fair proposal, but I'll freely admit the number of people actually auditing their code paths is orders of magnitude smaller than the number of users of the driver. There's such a large population of compatible legacy SoCs in the wild, adding an artificial boundary doesn't make sense. Especially since we're talking about features everyone would want to use. Perhaps we should keep both around, and deprecate the legacy driver over 3 to 4 cycles? But I guess that some users will want to use the new driver on the old marvell SoCs (especially kirkwood and dove). Yes, despite my arguments, I'm one of those people. :-P If we go to this path, then the best solution would be to still update all the the dts, and modifying the old driver to be able to use the new binding: for my point of view the only adaptation should be related to the SRAM. It will be also needed to find a way to be able to load only one driver at a time: either the old or the new, but not both. We can look at how the wireless drivers handle this. They often have to choose between multiple drivers (foss, proprietary, ndis-something, etc) for a given card. Not much different here. However I still wonder if it worth the effort. I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. thx,
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Jason, Jason Cooper ja...@lakedaemon.net writes: It's not about the crypto, it's about trust. imho, one of the most important security advances in the past 20 years is the default use of git (or other SCMs) by open source projects. Now, no one is forced to trust the authors and maintainers tarball dumps. Regular code audits and security updates are *much* more feasible because you can audit small changes. It can even be automated to a large extent. All this means the user has a choice: they can trust the authors and maintainers, or they can trust their own audits. Since updates are an essential part of a security posture, small commits facilitate maintaining the 'trust in audits'. It's not about Should you trust free-electrons? Or, Should you trust Jason / Herbert / Linus? It's about Should you have to trust any of them? It's ok, you can call our driver fat. It is ;-) More seriously, I tend to agree w/ what you write above. I really tried to adapt the existing driver to add the missing features (especially the support for TDMA), but all my attempts ended up introducing hackish code (not even talking about the performance penalty of this approach). Ok, fair enough. It would be helpful if this account of attempting to reconcile the old driver made it into the commit message. This puts us in perfect is the enemy of getting it done territory. I have another solution though: keep the existing driver for old marvell SoCs (orion, kirkwood and dove), and add a new one for modern SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver won't have to audit the new code. A fair proposal, but I'll freely admit the number of people actually auditing their code paths is orders of magnitude smaller than the number of users of the driver. There's such a large population of compatible legacy SoCs in the wild, adding an artificial boundary doesn't make sense. Especially since we're talking about features everyone would want to use. Perhaps we should keep both around, and deprecate the legacy driver over 3 to 4 cycles? But I guess that some users will want to use the new driver on the old marvell SoCs (especially kirkwood and dove). Yes, despite my arguments, I'm one of those people. :-P If we go to this path, then the best solution would be to still update all the the dts, and modifying the old driver to be able to use the new binding: for my point of view the only adaptation should be related to the SRAM. It will be also needed to find a way to be able to load only one driver at a time: either the old or the new, but not both. The approach Boris proposed above seems to make everyone happy: 1) Keep the old driver for old marvells SoCs (kirkwood, dove and orion) 2) Introduce the new driver for those that are not supported by the old driver, i.e. armada (370, XP, 375, 38x) AFAICT, this can easily be done (based on compatible strings) and it will let everyone the time to audit the new driver. Current users will not be taken by surprise. At some point, when everyone is confident w/ the new driver, we can then switch to that one for all SoCs so that old platform get more performance. Additionnally, for those who want to get the feature of the new driver for their old SoC right now, we *could* add a simple kernel config option for the new driver to use it for the old SoC too (that one disabling the old one). I'd appreciate if we'd look into it. I understand from on-list and off-list discussion that the rewrite was unavoidable. So I'm willing to concede that. Giving people time to migrate from old to new while still being able to update for other security fixes seems reasonable. Jason, what do you think of the approach above? Cheers, a+ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Boris, On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote: > On Fri, 10 Apr 2015 13:50:56 + Jason Cooper wrote: > > On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: > > > I know we usually try to adapt existing drivers instead of replacing them > > > by new ones, but after trying to refactor the mv_cesa driver I realized it > > > would take longer than writing an new one from scratch. > > > > I'm sorry, but this makes me *very* uncomfortable. Any organization > > worth it's salt will do a very careful audit of code touching > > cryptographic material and sensitive (decrypted) data. From that point > > on, small audits of the changes to the code allow the organization to > > build a comfort level with kernel updates. iow, following the git > > history of the driver. > > > > By apply this series, we are basically forcing those organizations to > > either a) stop updating, or b) expend significant resources to do > > another full audit. > > > > In short, this series breaks the audit chain for the mv_cesa driver. > > > > Maybe I'm the only person with this level of paranoia. If so, I'm sure > > others will override me. > > > > From my POV, it looks like the *only* reason we've chosen this route is > > developer convenience. I don't think that's sufficient reason to break > > the change history of a driver handling sensitive data. > > Well, I understand you concern, but if you read carefully the old and > new drivers, you'll notice that they are completely different (the only > thing I kept are the macro definitions). Yes, that's the worrying part for me. ;-) > I really tried to adapt the existing driver to add the missing > features (especially the support for TDMA), but all my attempts > ended up introducing hackish code (not even talking about the > performance penalty of this approach). Ok, fair enough. It would be helpful if this account of attempting to reconcile the old driver made it into the commit message. This puts us in "perfect is the enemy of getting it done" territory. > I have another solution though: keep the existing driver for old > marvell SoCs (orion, kirkwood and dove), and add a new one for modern > SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver > won't have to audit the new code. A fair proposal, but I'll freely admit the number of people actually auditing their code paths is orders of magnitude smaller than the number of users of the driver. There's such a large population of compatible legacy SoCs in the wild, adding an artificial boundary doesn't make sense. Especially since we're talking about features everyone would want to use. Perhaps we should keep both around, and deprecate the legacy driver over 3 to 4 cycles? thx, Jason. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hey Boris, On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote: On Fri, 10 Apr 2015 13:50:56 + Jason Cooper ja...@lakedaemon.net wrote: On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. I'm sorry, but this makes me *very* uncomfortable. Any organization worth it's salt will do a very careful audit of code touching cryptographic material and sensitive (decrypted) data. From that point on, small audits of the changes to the code allow the organization to build a comfort level with kernel updates. iow, following the git history of the driver. By apply this series, we are basically forcing those organizations to either a) stop updating, or b) expend significant resources to do another full audit. In short, this series breaks the audit chain for the mv_cesa driver. Maybe I'm the only person with this level of paranoia. If so, I'm sure others will override me. From my POV, it looks like the *only* reason we've chosen this route is developer convenience. I don't think that's sufficient reason to break the change history of a driver handling sensitive data. Well, I understand you concern, but if you read carefully the old and new drivers, you'll notice that they are completely different (the only thing I kept are the macro definitions). Yes, that's the worrying part for me. ;-) I really tried to adapt the existing driver to add the missing features (especially the support for TDMA), but all my attempts ended up introducing hackish code (not even talking about the performance penalty of this approach). Ok, fair enough. It would be helpful if this account of attempting to reconcile the old driver made it into the commit message. This puts us in perfect is the enemy of getting it done territory. I have another solution though: keep the existing driver for old marvell SoCs (orion, kirkwood and dove), and add a new one for modern SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver won't have to audit the new code. A fair proposal, but I'll freely admit the number of people actually auditing their code paths is orders of magnitude smaller than the number of users of the driver. There's such a large population of compatible legacy SoCs in the wild, adding an artificial boundary doesn't make sense. Especially since we're talking about features everyone would want to use. Perhaps we should keep both around, and deprecate the legacy driver over 3 to 4 cycles? thx, Jason. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Sebastian, On Thu, 09 Apr 2015 17:34:29 +0200 Sebastian Hesselbarth wrote: > > if you include a bunch of performance measurements, I guess it will help > you to get an agreement of replacing the driver instead of reworking it. Yep, I made some measurements using tcrypt a while ago, I'll provide them in the next round. > > > Here are the main features brought by this new driver: > > - support for armada SoCs (up to 38x) while keeping support for older ones > >(Orion and Kirkwood) > > Unfortunately, the list above is missing Dove SoCs which also have a > CESA engine with TDMA support. I checked the registers _very_ quickly > but it seems that they are compatible with Kirkwood's CESA. I checked it too: it should work, but I don't have any board to test it :-/. > > > - DMA mode to offload the CPU in case of intensive crypto usage > > - new algorithms: SHA256, DES and 3DES > > > [...] > > Boris Brezillon (2): > >crypto: add new driver for Marvell CESA > >crypto: marvell/CESA: update DT bindings documentation > > IMHO, the patch set should be split up in: > - new core driver > - add support for TDMA on platforms that support it > - new cipher algorithms > - removal of old mv_cesa I agree for the 2 steps operation: - add new driver code without compiling it - remove old code, update Kconfig (add new dependencies) and Makefile entries (compile the code in marvell/ instead of mv_cesa.c) Is there a good reason for separating the core, TDMA and algorithms support in different patches (keep Arnaud's authorship ?) ? Anyway, this should be feasible, but I thought the policy was to minimize the number of patches when submitting new drivers. > > I'd love to test on Dove, but time still is very limited. I guess the > patches will receive another round anyway, maybe I find some until the > final version. No problem (thanks for the offer). Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Am Donnerstag, 9. April 2015, 16:58:41 schrieb Boris Brezillon: Hi Boris, >Hello, > >This is an attempt to replace the mv_cesa driver by a new one to address >some limitations of the existing driver. >From a performance and CPU load point of view the most important >limitation is the lack of DMA support, thus preventing us from chaining >crypto operations. > >I know we usually try to adapt existing drivers instead of replacing them >by new ones, but after trying to refactor the mv_cesa driver I realized it >would take longer than writing an new one from scratch. > >Here are the main features brought by this new driver: >- support for armada SoCs (up to 38x) while keeping support for older ones > (Orion and Kirkwood) >- DMA mode to offload the CPU in case of intensive crypto usage >- new algorithms: SHA256, DES and 3DES > >I'd like to thank Arnaud, who has carefully reviewed several iterations of >this driver, helped me improved my implementation, provided support for >several crypto algorithms, provided support for armada-370 and tested >the driver on different platforms, hence the SoB and dual MODULE_AUTHOR >in the driver code. Your patch 1/2 did not make it to the crypto list. To big? It is on the lkml list though. > >Best Regards, > >Boris > >Boris Brezillon (2): > crypto: add new driver for Marvell CESA > crypto: marvell/CESA: update DT bindings documentation > > .../devicetree/bindings/crypto/mv_cesa.txt | 50 +- > drivers/crypto/Kconfig |2 + > drivers/crypto/Makefile|2 +- > drivers/crypto/marvell/Makefile|1 + > drivers/crypto/marvell/cesa.c | 539 > drivers/crypto/marvell/cesa.h | 802 > drivers/crypto/marvell/cipher.c| 761 +++ > drivers/crypto/marvell/hash.c | 1349 > drivers/crypto/marvell/tdma.c | >223 > drivers/crypto/mv_cesa.c | 1193 - > drivers/crypto/mv_cesa.h | 150 --- > 11 files changed, 3716 insertions(+), 1356 deletions(-) > create mode 100644 drivers/crypto/marvell/Makefile > create mode 100644 drivers/crypto/marvell/cesa.c > create mode 100644 drivers/crypto/marvell/cesa.h > create mode 100644 drivers/crypto/marvell/cipher.c > create mode 100644 drivers/crypto/marvell/hash.c > create mode 100644 drivers/crypto/marvell/tdma.c > delete mode 100644 drivers/crypto/mv_cesa.c > delete mode 100644 drivers/crypto/mv_cesa.h Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On 09.04.2015 16:58, Boris Brezillon wrote: This is an attempt to replace the mv_cesa driver by a new one to address some limitations of the existing driver. From a performance and CPU load point of view the most important limitation is the lack of DMA support, thus preventing us from chaining crypto operations. I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. Boris, if you include a bunch of performance measurements, I guess it will help you to get an agreement of replacing the driver instead of reworking it. Here are the main features brought by this new driver: - support for armada SoCs (up to 38x) while keeping support for older ones (Orion and Kirkwood) Unfortunately, the list above is missing Dove SoCs which also have a CESA engine with TDMA support. I checked the registers _very_ quickly but it seems that they are compatible with Kirkwood's CESA. - DMA mode to offload the CPU in case of intensive crypto usage - new algorithms: SHA256, DES and 3DES [...] Boris Brezillon (2): crypto: add new driver for Marvell CESA crypto: marvell/CESA: update DT bindings documentation IMHO, the patch set should be split up in: - new core driver - add support for TDMA on platforms that support it - new cipher algorithms - removal of old mv_cesa I'd love to test on Dove, but time still is very limited. I guess the patches will receive another round anyway, maybe I find some until the final version. Sebastian .../devicetree/bindings/crypto/mv_cesa.txt | 50 +- drivers/crypto/Kconfig |2 + drivers/crypto/Makefile|2 +- drivers/crypto/marvell/Makefile|1 + drivers/crypto/marvell/cesa.c | 539 drivers/crypto/marvell/cesa.h | 802 drivers/crypto/marvell/cipher.c| 761 +++ drivers/crypto/marvell/hash.c | 1349 drivers/crypto/marvell/tdma.c | 223 drivers/crypto/mv_cesa.c | 1193 - drivers/crypto/mv_cesa.h | 150 --- 11 files changed, 3716 insertions(+), 1356 deletions(-) create mode 100644 drivers/crypto/marvell/Makefile create mode 100644 drivers/crypto/marvell/cesa.c create mode 100644 drivers/crypto/marvell/cesa.h create mode 100644 drivers/crypto/marvell/cipher.c create mode 100644 drivers/crypto/marvell/hash.c create mode 100644 drivers/crypto/marvell/tdma.c delete mode 100644 drivers/crypto/mv_cesa.c delete mode 100644 drivers/crypto/mv_cesa.h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: > Hello, > > This is an attempt to replace the mv_cesa driver by a new one to address > some limitations of the existing driver. > >From a performance and CPU load point of view the most important > limitation is the lack of DMA support, thus preventing us from chaining > crypto operations. > > I know we usually try to adapt existing drivers instead of replacing them > by new ones, but after trying to refactor the mv_cesa driver I realized it > would take longer than writing an new one from scratch. Hi Boris What is the situation with backwards compatibility? I see you have kept the old compatibility string, and added lots of new ones, and deprecated some properties. Will an old DT blob still work? Thanks Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Hi Sebastian, On Thu, 09 Apr 2015 17:34:29 +0200 Sebastian Hesselbarth sebastian.hesselba...@gmail.com wrote: if you include a bunch of performance measurements, I guess it will help you to get an agreement of replacing the driver instead of reworking it. Yep, I made some measurements using tcrypt a while ago, I'll provide them in the next round. Here are the main features brought by this new driver: - support for armada SoCs (up to 38x) while keeping support for older ones (Orion and Kirkwood) Unfortunately, the list above is missing Dove SoCs which also have a CESA engine with TDMA support. I checked the registers _very_ quickly but it seems that they are compatible with Kirkwood's CESA. I checked it too: it should work, but I don't have any board to test it :-/. - DMA mode to offload the CPU in case of intensive crypto usage - new algorithms: SHA256, DES and 3DES [...] Boris Brezillon (2): crypto: add new driver for Marvell CESA crypto: marvell/CESA: update DT bindings documentation IMHO, the patch set should be split up in: - new core driver - add support for TDMA on platforms that support it - new cipher algorithms - removal of old mv_cesa I agree for the 2 steps operation: - add new driver code without compiling it - remove old code, update Kconfig (add new dependencies) and Makefile entries (compile the code in marvell/ instead of mv_cesa.c) Is there a good reason for separating the core, TDMA and algorithms support in different patches (keep Arnaud's authorship ?) ? Anyway, this should be feasible, but I thought the policy was to minimize the number of patches when submitting new drivers. I'd love to test on Dove, but time still is very limited. I guess the patches will receive another round anyway, maybe I find some until the final version. No problem (thanks for the offer). Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote: Hello, This is an attempt to replace the mv_cesa driver by a new one to address some limitations of the existing driver. From a performance and CPU load point of view the most important limitation is the lack of DMA support, thus preventing us from chaining crypto operations. I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. Hi Boris What is the situation with backwards compatibility? I see you have kept the old compatibility string, and added lots of new ones, and deprecated some properties. Will an old DT blob still work? Thanks Andrew -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
On 09.04.2015 16:58, Boris Brezillon wrote: This is an attempt to replace the mv_cesa driver by a new one to address some limitations of the existing driver. From a performance and CPU load point of view the most important limitation is the lack of DMA support, thus preventing us from chaining crypto operations. I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. Boris, if you include a bunch of performance measurements, I guess it will help you to get an agreement of replacing the driver instead of reworking it. Here are the main features brought by this new driver: - support for armada SoCs (up to 38x) while keeping support for older ones (Orion and Kirkwood) Unfortunately, the list above is missing Dove SoCs which also have a CESA engine with TDMA support. I checked the registers _very_ quickly but it seems that they are compatible with Kirkwood's CESA. - DMA mode to offload the CPU in case of intensive crypto usage - new algorithms: SHA256, DES and 3DES [...] Boris Brezillon (2): crypto: add new driver for Marvell CESA crypto: marvell/CESA: update DT bindings documentation IMHO, the patch set should be split up in: - new core driver - add support for TDMA on platforms that support it - new cipher algorithms - removal of old mv_cesa I'd love to test on Dove, but time still is very limited. I guess the patches will receive another round anyway, maybe I find some until the final version. Sebastian .../devicetree/bindings/crypto/mv_cesa.txt | 50 +- drivers/crypto/Kconfig |2 + drivers/crypto/Makefile|2 +- drivers/crypto/marvell/Makefile|1 + drivers/crypto/marvell/cesa.c | 539 drivers/crypto/marvell/cesa.h | 802 drivers/crypto/marvell/cipher.c| 761 +++ drivers/crypto/marvell/hash.c | 1349 drivers/crypto/marvell/tdma.c | 223 drivers/crypto/mv_cesa.c | 1193 - drivers/crypto/mv_cesa.h | 150 --- 11 files changed, 3716 insertions(+), 1356 deletions(-) create mode 100644 drivers/crypto/marvell/Makefile create mode 100644 drivers/crypto/marvell/cesa.c create mode 100644 drivers/crypto/marvell/cesa.h create mode 100644 drivers/crypto/marvell/cipher.c create mode 100644 drivers/crypto/marvell/hash.c create mode 100644 drivers/crypto/marvell/tdma.c delete mode 100644 drivers/crypto/mv_cesa.c delete mode 100644 drivers/crypto/mv_cesa.h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] crypto: add new driver for Marvell CESA
Am Donnerstag, 9. April 2015, 16:58:41 schrieb Boris Brezillon: Hi Boris, Hello, This is an attempt to replace the mv_cesa driver by a new one to address some limitations of the existing driver. From a performance and CPU load point of view the most important limitation is the lack of DMA support, thus preventing us from chaining crypto operations. I know we usually try to adapt existing drivers instead of replacing them by new ones, but after trying to refactor the mv_cesa driver I realized it would take longer than writing an new one from scratch. Here are the main features brought by this new driver: - support for armada SoCs (up to 38x) while keeping support for older ones (Orion and Kirkwood) - DMA mode to offload the CPU in case of intensive crypto usage - new algorithms: SHA256, DES and 3DES I'd like to thank Arnaud, who has carefully reviewed several iterations of this driver, helped me improved my implementation, provided support for several crypto algorithms, provided support for armada-370 and tested the driver on different platforms, hence the SoB and dual MODULE_AUTHOR in the driver code. Your patch 1/2 did not make it to the crypto list. To big? It is on the lkml list though. Best Regards, Boris Boris Brezillon (2): crypto: add new driver for Marvell CESA crypto: marvell/CESA: update DT bindings documentation .../devicetree/bindings/crypto/mv_cesa.txt | 50 +- drivers/crypto/Kconfig |2 + drivers/crypto/Makefile|2 +- drivers/crypto/marvell/Makefile|1 + drivers/crypto/marvell/cesa.c | 539 drivers/crypto/marvell/cesa.h | 802 drivers/crypto/marvell/cipher.c| 761 +++ drivers/crypto/marvell/hash.c | 1349 drivers/crypto/marvell/tdma.c | 223 drivers/crypto/mv_cesa.c | 1193 - drivers/crypto/mv_cesa.h | 150 --- 11 files changed, 3716 insertions(+), 1356 deletions(-) create mode 100644 drivers/crypto/marvell/Makefile create mode 100644 drivers/crypto/marvell/cesa.c create mode 100644 drivers/crypto/marvell/cesa.h create mode 100644 drivers/crypto/marvell/cipher.c create mode 100644 drivers/crypto/marvell/hash.c create mode 100644 drivers/crypto/marvell/tdma.c delete mode 100644 drivers/crypto/mv_cesa.c delete mode 100644 drivers/crypto/mv_cesa.h Ciao Stephan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
