The only caller of hisi_zip_vf_q_assign() is hidden in an #ifdef,
so the function causes a warning when CONFIG_PCI_IOV is disabled:
drivers/crypto/hisilicon/zip/zip_main.c:740:12: error: unused function 'hisi_zip_vf_q_assign' [-Werror,-Wunused-function]
Replace the #ifdef with an IS_ENABLED() check that leads to the
function being dropped based on the configuration.
Fixes: 79e09f30eeba ("crypto: hisilicon - add SRIOV support for ZIP")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/crypto/hisilicon/zip/zip_main.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/crypto/hisilicon/zip/zip_main.c b/drivers/crypto/hisilicon/zip/zip_main.c
index 6e0ca75585d4..1b2ee96c888d 100644
--- a/drivers/crypto/hisilicon/zip/zip_main.c
+++ b/drivers/crypto/hisilicon/zip/zip_main.c
@@ -785,7 +785,6 @@ static int hisi_zip_clear_vft_config(struct hisi_zip *hisi_zip)
static int hisi_zip_sriov_enable(struct pci_dev *pdev, int max_vfs)
{
-#ifdef CONFIG_PCI_IOV
struct hisi_zip *hisi_zip = pci_get_drvdata(pdev);
int pre_existing_vfs, num_vfs, ret;
@@ -815,9 +814,6 @@ static int hisi_zip_sriov_enable(struct pci_dev *pdev, int max_vfs)
}
return num_vfs;
-#else
- return 0;
-#endif
}
static int hisi_zip_sriov_disable(struct pci_dev *pdev)
@@ -948,7 +944,8 @@ static struct pci_driver hisi_zip_pci_driver = {
.id_table = hisi_zip_dev_ids,
.probe = hisi_zip_probe,
.remove = hisi_zip_remove,
- .sriov_configure = hisi_zip_sriov_configure,
+ .sriov_configure = IS_ENABLED(CONFIG_PCI_IOV) ?
+ hisi_zip_sriov_configure : 0,
.err_handler = &hisi_zip_err_handler,
};
--
2.20.0
To avoid missing arm64 specific warnings that get introduced
in this driver, allow compile-testing on all 64-bit architectures.
The only actual arm64 specific code in this driver is an open-
coded 128 bit MMIO write. On non-arm64 the same can be done
using memcpy_toio. What I also noticed is that the mmio store
(either one) is not endian-safe, this will only work on little-
endian configurations, so I also add a Kconfig dependency on
that, regardless of the architecture.
Finally, a depenndecy on CONFIG_64BIT is needed because of the
writeq().
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/crypto/hisilicon/Kconfig | 8 +++++---
drivers/crypto/hisilicon/qm.c | 6 ++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
index ebaf91e0146d..dfbd668a431e 100644
--- a/drivers/crypto/hisilicon/Kconfig
+++ b/drivers/crypto/hisilicon/Kconfig
@@ -16,14 +16,15 @@ config CRYPTO_DEV_HISI_SEC
config CRYPTO_DEV_HISI_QM
tristate
- depends on ARM64 && PCI && PCI_MSI
+ depends on ARM64 || COMPILE_TEST
+ depends on PCI && PCI_MSI
help
HiSilicon accelerator engines use a common queue management
interface. Specific engine driver may use this module.
config CRYPTO_HISI_SGL
tristate
- depends on ARM64
+ depends on ARM64 || COMPILE_TEST
help
HiSilicon accelerator engines use a common hardware scatterlist
interface for data format. Specific engine driver may use this
@@ -31,7 +32,8 @@ config CRYPTO_HISI_SGL
config CRYPTO_DEV_HISI_ZIP
tristate "Support for HiSilicon ZIP accelerator"
- depends on ARM64 && PCI && PCI_MSI
+ depends on PCI && PCI_MSI
+ depends on ARM64 || (COMPILE_TEST && 64BIT)
select CRYPTO_DEV_HISI_QM
select CRYPTO_HISI_SGL
select SG_SPLIT
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index f975c393a603..a8ed699081b7 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -331,6 +331,12 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
unsigned long tmp0 = 0, tmp1 = 0;
+ if (!IS_ENABLED(CONFIG_ARM64)) {
+ memcpy_toio(fun_base, src, 16);
+ wmb();
+ return;
+ }
+
asm volatile("ldp %0, %1, %3\n"
"stp %0, %1, %2\n"
"dsb sy\n"
--
2.20.0
To avoid missing arm64 specific warnings that get introduced
in this driver, allow compile-testing on all 64-bit architectures.
The only actual arm64 specific code in this driver is an open-
coded 128 bit MMIO write. On non-arm64 the same can be done
using memcpy_toio. What I also noticed is that the mmio store
(either one) is not endian-safe, this will only work on little-
endian configurations, so I also add a Kconfig dependency on
that, regardless of the architecture.
Finally, a depenndecy on CONFIG_64BIT is needed because of the
writeq().
Signed-off-by: Arnd Bergmann <[email protected]>
---
v2: actually add !CPU_BIG_ENDIAN dependency as described in the
changelog
---
drivers/crypto/hisilicon/Kconfig | 9 ++++++---
drivers/crypto/hisilicon/qm.c | 6 ++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
index ebaf91e0146d..7bfcaa7674fd 100644
--- a/drivers/crypto/hisilicon/Kconfig
+++ b/drivers/crypto/hisilicon/Kconfig
@@ -16,14 +16,15 @@ config CRYPTO_DEV_HISI_SEC
config CRYPTO_DEV_HISI_QM
tristate
- depends on ARM64 && PCI && PCI_MSI
+ depends on ARM64 || COMPILE_TEST
+ depends on PCI && PCI_MSI
help
HiSilicon accelerator engines use a common queue management
interface. Specific engine driver may use this module.
config CRYPTO_HISI_SGL
tristate
- depends on ARM64
+ depends on ARM64 || COMPILE_TEST
help
HiSilicon accelerator engines use a common hardware scatterlist
interface for data format. Specific engine driver may use this
@@ -31,7 +32,9 @@ config CRYPTO_HISI_SGL
config CRYPTO_DEV_HISI_ZIP
tristate "Support for HiSilicon ZIP accelerator"
- depends on ARM64 && PCI && PCI_MSI
+ depends on PCI && PCI_MSI
+ depends on ARM64 || (COMPILE_TEST && 64BIT)
+ depends on !CPU_BIG_ENDIAN || COMPILE_TEST
select CRYPTO_DEV_HISI_QM
select CRYPTO_HISI_SGL
select SG_SPLIT
diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
index f975c393a603..a8ed699081b7 100644
--- a/drivers/crypto/hisilicon/qm.c
+++ b/drivers/crypto/hisilicon/qm.c
@@ -331,6 +331,12 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
unsigned long tmp0 = 0, tmp1 = 0;
+ if (!IS_ENABLED(CONFIG_ARM64)) {
+ memcpy_toio(fun_base, src, 16);
+ wmb();
+ return;
+ }
+
asm volatile("ldp %0, %1, %3\n"
"stp %0, %1, %2\n"
"dsb sy\n"
--
2.20.0
On 19/09/2019 15:09, Arnd Bergmann wrote:
> To avoid missing arm64 specific warnings that get introduced
> in this driver, allow compile-testing on all 64-bit architectures.
>
> The only actual arm64 specific code in this driver is an open-
> coded 128 bit MMIO write. On non-arm64 the same can be done
> using memcpy_toio. What I also noticed is that the mmio store
> (either one) is not endian-safe, this will only work on little-
> endian configurations, so I also add a Kconfig dependency on
> that, regardless of the architecture.
> Finally, a depenndecy on CONFIG_64BIT is needed because of the
nit: spelling mistake
> writeq().
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> v2: actually add !CPU_BIG_ENDIAN dependency as described in the
> changelog
> ---
> drivers/crypto/hisilicon/Kconfig | 9 ++++++---
> drivers/crypto/hisilicon/qm.c | 6 ++++++
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/hisilicon/Kconfig b/drivers/crypto/hisilicon/Kconfig
> index ebaf91e0146d..7bfcaa7674fd 100644
> --- a/drivers/crypto/hisilicon/Kconfig
> +++ b/drivers/crypto/hisilicon/Kconfig
> @@ -16,14 +16,15 @@ config CRYPTO_DEV_HISI_SEC
>
> config CRYPTO_DEV_HISI_QM
> tristate
> - depends on ARM64 && PCI && PCI_MSI
> + depends on ARM64 || COMPILE_TEST
> + depends on PCI && PCI_MSI
> help
> HiSilicon accelerator engines use a common queue management
> interface. Specific engine driver may use this module.
>
> config CRYPTO_HISI_SGL
> tristate
> - depends on ARM64
> + depends on ARM64 || COMPILE_TEST
> help
> HiSilicon accelerator engines use a common hardware scatterlist
> interface for data format. Specific engine driver may use this
> @@ -31,7 +32,9 @@ config CRYPTO_HISI_SGL
>
> config CRYPTO_DEV_HISI_ZIP
> tristate "Support for HiSilicon ZIP accelerator"
> - depends on ARM64 && PCI && PCI_MSI
> + depends on PCI && PCI_MSI
> + depends on ARM64 || (COMPILE_TEST && 64BIT)
> + depends on !CPU_BIG_ENDIAN || COMPILE_TEST
> select CRYPTO_DEV_HISI_QM
> select CRYPTO_HISI_SGL
> select SG_SPLIT
> diff --git a/drivers/crypto/hisilicon/qm.c b/drivers/crypto/hisilicon/qm.c
> index f975c393a603..a8ed699081b7 100644
> --- a/drivers/crypto/hisilicon/qm.c
> +++ b/drivers/crypto/hisilicon/qm.c
> @@ -331,6 +331,12 @@ static void qm_mb_write(struct hisi_qm *qm, const void *src)
> void __iomem *fun_base = qm->io_base + QM_MB_CMD_SEND_BASE;
> unsigned long tmp0 = 0, tmp1 = 0;
>
Hi Arnd,
> + if (!IS_ENABLED(CONFIG_ARM64)) {
> + memcpy_toio(fun_base, src, 16);
> + wmb();
> + return;
> + }
> +
> asm volatile("ldp %0, %1, %3\n"
> "stp %0, %1, %2\n"
> "dsb sy\n"
>
As I understand, this operation needs to be done atomically. So - even
though your change is just for compile testing - the memcpy_to_io() may
not do the same thing on other archs, right?
I just wonder if it's right to make that change, or at least warn the
imaginary user of possible malfunction for !arm64.
Thanks,
John
On Thu, Sep 19, 2019 at 04:05:52PM +0200, Arnd Bergmann wrote:
> The only caller of hisi_zip_vf_q_assign() is hidden in an #ifdef,
> so the function causes a warning when CONFIG_PCI_IOV is disabled:
>
> drivers/crypto/hisilicon/zip/zip_main.c:740:12: error: unused function 'hisi_zip_vf_q_assign' [-Werror,-Wunused-function]
>
> Replace the #ifdef with an IS_ENABLED() check that leads to the
> function being dropped based on the configuration.
>
> Fixes: 79e09f30eeba ("crypto: hisilicon - add SRIOV support for ZIP")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/crypto/hisilicon/zip/zip_main.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, Sep 20, 2019 at 10:34 AM John Garry <[email protected]> wrote:
> > + if (!IS_ENABLED(CONFIG_ARM64)) {
> > + memcpy_toio(fun_base, src, 16);
> > + wmb();
> > + return;
> > + }
> > +
> > asm volatile("ldp %0, %1, %3\n"
> > "stp %0, %1, %2\n"
> > "dsb sy\n"
> >
>
> As I understand, this operation needs to be done atomically. So - even
> though your change is just for compile testing - the memcpy_to_io() may
> not do the same thing on other archs, right?
>
> I just wonder if it's right to make that change, or at least warn the
> imaginary user of possible malfunction for !arm64.
It's probably not necessary here. From what I can tell from the documentation,
this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
don't guarantee that an stp arrives on the bus in one piece either.
Usually, hardware like this has no hard requirement on an atomic store,
it just needs the individual bits to arrive in a particular order, and then
triggers the update on the last bit that gets stored. If that is the case here
as well, it might actually be better to use two writeq_relaxed() and
a barrier. This would also solve the endianess issue.
Arnd
On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <[email protected]> wrote:
>
> On Fri, Sep 20, 2019 at 10:34 AM John Garry <[email protected]> wrote:
>
> > > + if (!IS_ENABLED(CONFIG_ARM64)) {
> > > + memcpy_toio(fun_base, src, 16);
> > > + wmb();
> > > + return;
> > > + }
> > > +
> > > asm volatile("ldp %0, %1, %3\n"
> > > "stp %0, %1, %2\n"
> > > "dsb sy\n"
> > >
> >
> > As I understand, this operation needs to be done atomically. So - even
> > though your change is just for compile testing - the memcpy_to_io() may
> > not do the same thing on other archs, right?
> >
> > I just wonder if it's right to make that change, or at least warn the
> > imaginary user of possible malfunction for !arm64.
>
> It's probably not necessary here. From what I can tell from the documentation,
> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
> don't guarantee that an stp arrives on the bus in one piece either.
>
> Usually, hardware like this has no hard requirement on an atomic store,
> it just needs the individual bits to arrive in a particular order, and then
> triggers the update on the last bit that gets stored. If that is the case here
> as well, it might actually be better to use two writeq_relaxed() and
> a barrier. This would also solve the endianess issue.
See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
to introduce 128-bit MMIO accessors, this got rejected since they
are not atomic even on ARMv8.4.
Arnd
On 20/09/2019 14:36, Arnd Bergmann wrote:
> On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <[email protected]> wrote:
>>
>> On Fri, Sep 20, 2019 at 10:34 AM John Garry <[email protected]> wrote:
>>
>>>> + if (!IS_ENABLED(CONFIG_ARM64)) {
>>>> + memcpy_toio(fun_base, src, 16);
>>>> + wmb();
>>>> + return;
>>>> + }
>>>> +
>>>> asm volatile("ldp %0, %1, %3\n"
>>>> "stp %0, %1, %2\n"
>>>> "dsb sy\n"
>>>>
>>>
>>> As I understand, this operation needs to be done atomically. So - even
>>> though your change is just for compile testing - the memcpy_to_io() may
>>> not do the same thing on other archs, right?
>>>
>>> I just wonder if it's right to make that change, or at least warn the
>>> imaginary user of possible malfunction for !arm64.
>>
Hi Arnd,
>> It's probably not necessary here. From what I can tell from the documentation,
>> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
>> don't guarantee that an stp arrives on the bus in one piece either.
>>
>> Usually, hardware like this has no hard requirement on an atomic store,
>> it just needs the individual bits to arrive in a particular order, and then
>> triggers the update on the last bit that gets stored. If that is the case here
>> as well, it might actually be better to use two writeq_relaxed() and
>> a barrier. This would also solve the endianess issue.
>
> See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
> to introduce 128-bit MMIO accessors, this got rejected since they
> are not atomic even on ARMv8.4.
So this is proprietary IP integrated with a proprietary ARMv8
implementation, so there could be a tight coupling, the like of which
Will mentioned in that thread, but I'm doubtful.
I'm looking at the electronically translated documentation on this HW,
and it reads "The Mailbox operation performed by the CPU cannot be
interleaved", and then tells that software should lock against
concurrent accesses or alternatively use a 128-bit access. So it seems
that the 128b op used is only to guarantee software is atomic.
Wang Zhou can confirm my understanding.
If true, I see that we seem to be already guaranteeing mutual exclusion
in qm_mb(), in taking a mutex.
Thanks,
John
>
> Arnd
>
> .
>
On 2019/9/20 22:16, John Garry wrote:
> On 20/09/2019 14:36, Arnd Bergmann wrote:
>> On Fri, Sep 20, 2019 at 3:26 PM Arnd Bergmann <[email protected]> wrote:
>>>
>>> On Fri, Sep 20, 2019 at 10:34 AM John Garry <[email protected]> wrote:
>>>
>>>>> + if (!IS_ENABLED(CONFIG_ARM64)) {
>>>>> + memcpy_toio(fun_base, src, 16);
>>>>> + wmb();
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> asm volatile("ldp %0, %1, %3\n"
>>>>> "stp %0, %1, %2\n"
>>>>> "dsb sy\n"
>>>>>
>>>>
>>>> As I understand, this operation needs to be done atomically. So - even
>>>> though your change is just for compile testing - the memcpy_to_io() may
>>>> not do the same thing on other archs, right?
>>>>
>>>> I just wonder if it's right to make that change, or at least warn the
>>>> imaginary user of possible malfunction for !arm64.
>>>
>
> Hi Arnd,
>
>>> It's probably not necessary here. From what I can tell from the documentation,
>>> this is only safe on ARMv8.4 or higher anyway, earlier ARMv8.x implementations
>>> don't guarantee that an stp arrives on the bus in one piece either.
>>>
>>> Usually, hardware like this has no hard requirement on an atomic store,
>>> it just needs the individual bits to arrive in a particular order, and then
>>> triggers the update on the last bit that gets stored. If that is the case here
>>> as well, it might actually be better to use two writeq_relaxed() and
>>> a barrier. This would also solve the endianess issue.
>>
>> See also https://lkml.org/lkml/2018/1/26/554 for a previous attempt
>> to introduce 128-bit MMIO accessors, this got rejected since they
>> are not atomic even on ARMv8.4.
>
> So this is proprietary IP integrated with a proprietary ARMv8 implementation,
> so there could be a tight coupling, the like of which Will mentioned in that thread,
> but I'm doubtful.
>
> I'm looking at the electronically translated documentation on this HW, and it reads
> "The Mailbox operation performed by the CPU cannot be interleaved", and then tells
> that software should lock against concurrent accesses or alternatively use a 128-bit
> access. So it seems that the 128b op used is only to guarantee software is atomic.
>
> Wang Zhou can confirm my understanding
We have to do a 128bit atomic write here to trigger a mailbox. The reason is
that one QM hardware entity in one accelerator servers QM mailbox MMIO interfaces in
related PF and VFs.
A mutex can not lock different processing flows in different functions.
As Arnd mentioned, v8.4 extends the support for 16 bytes atomic stp to some kinds of
normal memory, but for device memory, it is still implementation defined. For this
SoC(Kunpeng920) which has QM/ZIP, if the address is 128bit aligned, stp will be atomic.
The offset of QM mailbox is 128bit aligned, so it is safe here.
Best,
Zhou
>
> If true, I see that we seem to be already guaranteeing mutual exclusion in qm_mb(),
> in taking a mutex.
>
> Thanks,
> John
>
>
>>
>> Arnd
>>
>> .
>>
>
>
>
> .
>
On Thu, Sep 19, 2019 at 04:09:06PM +0200, Arnd Bergmann wrote:
> To avoid missing arm64 specific warnings that get introduced
> in this driver, allow compile-testing on all 64-bit architectures.
>
> The only actual arm64 specific code in this driver is an open-
> coded 128 bit MMIO write. On non-arm64 the same can be done
> using memcpy_toio. What I also noticed is that the mmio store
> (either one) is not endian-safe, this will only work on little-
> endian configurations, so I also add a Kconfig dependency on
> that, regardless of the architecture.
> Finally, a depenndecy on CONFIG_64BIT is needed because of the
> writeq().
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> v2: actually add !CPU_BIG_ENDIAN dependency as described in the
> changelog
> ---
> drivers/crypto/hisilicon/Kconfig | 9 ++++++---
> drivers/crypto/hisilicon/qm.c | 6 ++++++
> 2 files changed, 12 insertions(+), 3 deletions(-)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt