On Fri, Mar 31, 2017 at 10:23 AM, Horia Geantă <[email protected]> wrote:
> On 3/31/2017 1:40 PM, Russell King - ARM Linux wrote:
>> Ping, this issue still exists with 4.11-rc4 - and there's been no
>> reaction from the alleged CAAM maintainers.
>>
> Sorry, this somehow slipped through (Cc vs. To, no linux-crypto).
>
>> On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote:
>>> On Tue, Aug 9, 2016 at 4:33 AM, Russell King <[email protected]> wrote:
>>>> When a Linux device is released and cleaned up, we left the OF device
>>>> node marked as populated. This causes the Freescale CAAM driver
>>>> (drivers/crypto/caam) problems when the module is removed and re-
>>>> inserted:
>>>>
>>>> JR0 Platform device creation error
>>>> JR0 Platform device creation error
>>>> caam 2100000.caam: no queues configured, terminating
>>>> caam: probe of 2100000.caam failed with error -12
>>>>
>>>> The reason is that CAAM creates platform devices for each job ring:
>>>>
>>>> for_each_available_child_of_node(nprop, np)
>>>> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
>>>> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) {
>>>> ctrlpriv->jrpdev[ring] =
>>>> of_platform_device_create(np, NULL, dev);
>>>>
>>>> which sets OF_POPULATED on the device node, but then it cleans these
>>>> up:
>>>>
>>>> /* Remove platform devices for JobRs */
>>>> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) {
>>>> if (ctrlpriv->jrpdev[ring])
>>>> of_device_unregister(ctrlpriv->jrpdev[ring]);
>>>
>>> This looks a bit asymmetrical to me with a of_platform_device_* call
>>> and a of_device_* call.
>>>
>>> I think you could use of_platform_{de}populate here instead. That
>>> would simplify things in the driver a bit too as you wouldn't need to
>>> store jrpdev. It wouldn't work if there are other child nodes with
> Indeed, this would clean-up the driver a bit. However, the driver needs
> to know how many of the devices probed successfully - to print the
> number and more importantly to exit in case total_jobrs = 0.
The only thing you are guaranteed is the OF code created some platform
devices. That's it. Whether any driver probed successfully is separate
and a lot more things can go wrong there. The only thing you are
checking is that your dtb is not crap.
> Thus, I would keep the one-by-one probing of the devices.
> What options are there in this case?
> Should a function symmetric to of_platform_device_create() be added - to
> replace of_device_unregister() - or rely on an open-coded solution?
Certainly not the latter. We don't want drivers mucking with flags
internal to the DT code.
Rob
The way Job Ring platform devices are created and released does not
allow for multiple create-release cycles.
JR0 Platform device creation error
JR0 Platform device creation error
caam 2100000.caam: no queues configured, terminating
caam: probe of 2100000.caam failed with error -12
The reason is that platform devices are created for each job ring:
for_each_available_child_of_node(nprop, np)
if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
of_device_is_compatible(np, "fsl,sec4.0-job-ring")) {
ctrlpriv->jrpdev[ring] =
of_platform_device_create(np, NULL, dev);
which sets OF_POPULATED on the device node, but then it cleans these up:
/* Remove platform devices for JobRs */
for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) {
if (ctrlpriv->jrpdev[ring])
of_device_unregister(ctrlpriv->jrpdev[ring]);
}
which leaves OF_POPULATED set.
Use of_platform_populate / of_platform_depopulate instead.
This allows for a bit of driver clean-up, jrpdev is no longer needed.
Logic changes a bit too:
-exit in case of_platform_populate fails, since currently even QI backend
depends on JR; true, we no longer support the case when "some" of the JR
DT nodes are incorrect
-when cleaning up, caam_remove() would also depopulate RTIC in case
it would have been populated somewhere else - not the case for now
Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring")
Reported-by: Russell King <[email protected]>
Suggested-by: Rob Herring <[email protected]>
Signed-off-by: Horia Geantă <[email protected]>
---
Not sending this directly to -stable, since it does not apply cleanly.
drivers/crypto/caam/ctrl.c | 64 ++++++++++++++------------------------------
drivers/crypto/caam/intern.h | 1 -
2 files changed, 20 insertions(+), 45 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index b3a94d5eff26..f7792a99469a 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -305,15 +305,13 @@ static int caam_remove(struct platform_device *pdev)
struct device *ctrldev;
struct caam_drv_private *ctrlpriv;
struct caam_ctrl __iomem *ctrl;
- int ring;
ctrldev = &pdev->dev;
ctrlpriv = dev_get_drvdata(ctrldev);
ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
- /* Remove platform devices for JobRs */
- for (ring = 0; ring < ctrlpriv->total_jobrs; ring++)
- of_device_unregister(ctrlpriv->jrpdev[ring]);
+ /* Remove platform devices under the crypto node */
+ of_platform_depopulate(ctrldev);
#ifdef CONFIG_CAAM_QI
if (ctrlpriv->qidev)
@@ -410,10 +408,21 @@ int caam_get_era(void)
}
EXPORT_SYMBOL(caam_get_era);
+static const struct of_device_id caam_match[] = {
+ {
+ .compatible = "fsl,sec-v4.0",
+ },
+ {
+ .compatible = "fsl,sec4.0",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, caam_match);
+
/* Probe routine for CAAM top (controller) level */
static int caam_probe(struct platform_device *pdev)
{
- int ret, ring, ridx, rspec, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
+ int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
u64 caam_id;
struct device *dev;
struct device_node *nprop, *np;
@@ -589,21 +598,9 @@ static int caam_probe(struct platform_device *pdev)
goto iounmap_ctrl;
}
- /*
- * Detect and enable JobRs
- * First, find out how many ring spec'ed, allocate references
- * for all, then go probe each one.
- */
- rspec = 0;
- for_each_available_child_of_node(nprop, np)
- if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
- of_device_is_compatible(np, "fsl,sec4.0-job-ring"))
- rspec++;
-
- ctrlpriv->jrpdev = devm_kcalloc(&pdev->dev, rspec,
- sizeof(*ctrlpriv->jrpdev), GFP_KERNEL);
- if (ctrlpriv->jrpdev == NULL) {
- ret = -ENOMEM;
+ ret = of_platform_populate(nprop, caam_match, NULL, dev);
+ if (ret) {
+ dev_err(dev, "JR platform devices creation error\n");
goto iounmap_ctrl;
}
@@ -618,29 +615,19 @@ static int caam_probe(struct platform_device *pdev)
ctrlpriv->dfs_root = debugfs_create_dir(dev_name(dev), NULL);
ctrlpriv->ctl = debugfs_create_dir("ctl", ctrlpriv->dfs_root);
#endif
+
ring = 0;
- ridx = 0;
- ctrlpriv->total_jobrs = 0;
for_each_available_child_of_node(nprop, np)
if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
of_device_is_compatible(np, "fsl,sec4.0-job-ring")) {
- ctrlpriv->jrpdev[ring] =
- of_platform_device_create(np, NULL, dev);
- if (!ctrlpriv->jrpdev[ring]) {
- pr_warn("JR physical index %d: Platform device creation error\n",
- ridx);
- ridx++;
- continue;
- }
ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *)
((__force uint8_t *)ctrl +
- (ridx + JR_BLOCK_NUMBER) *
+ (ring + JR_BLOCK_NUMBER) *
BLOCK_OFFSET
);
ctrlpriv->total_jobrs++;
ring++;
- ridx++;
- }
+ }
/* Check to see if QI present. If so, enable */
ctrlpriv->qi_present =
@@ -849,17 +836,6 @@ static int caam_probe(struct platform_device *pdev)
return ret;
}
-static struct of_device_id caam_match[] = {
- {
- .compatible = "fsl,sec-v4.0",
- },
- {
- .compatible = "fsl,sec4.0",
- },
- {},
-};
-MODULE_DEVICE_TABLE(of, caam_match);
-
static struct platform_driver caam_driver = {
.driver = {
.name = "caam",
diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h
index c334df638ff6..85b6c5835b8f 100644
--- a/drivers/crypto/caam/intern.h
+++ b/drivers/crypto/caam/intern.h
@@ -66,7 +66,6 @@ struct caam_drv_private_jr {
struct caam_drv_private {
struct device *dev;
- struct platform_device **jrpdev; /* Alloc'ed array per sub-device */
#ifdef CONFIG_CAAM_QI
struct device *qidev;
#endif
--
2.12.0.264.gd6db3f216544
On Mon, Apr 3, 2017 at 10:12 AM, Horia Geantă <[email protected]> wrote:
> The way Job Ring platform devices are created and released does not
> allow for multiple create-release cycles.
>
> JR0 Platform device creation error
> JR0 Platform device creation error
> caam 2100000.caam: no queues configured, terminating
> caam: probe of 2100000.caam failed with error -12
>
> The reason is that platform devices are created for each job ring:
>
> for_each_available_child_of_node(nprop, np)
> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) {
> ctrlpriv->jrpdev[ring] =
> of_platform_device_create(np, NULL, dev);
>
> which sets OF_POPULATED on the device node, but then it cleans these up:
>
> /* Remove platform devices for JobRs */
> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) {
> if (ctrlpriv->jrpdev[ring])
> of_device_unregister(ctrlpriv->jrpdev[ring]);
> }
>
> which leaves OF_POPULATED set.
>
> Use of_platform_populate / of_platform_depopulate instead.
> This allows for a bit of driver clean-up, jrpdev is no longer needed.
>
> Logic changes a bit too:
> -exit in case of_platform_populate fails, since currently even QI backend
> depends on JR; true, we no longer support the case when "some" of the JR
> DT nodes are incorrect
> -when cleaning up, caam_remove() would also depopulate RTIC in case
> it would have been populated somewhere else - not the case for now
>
> Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring")
> Reported-by: Russell King <[email protected]>
> Suggested-by: Rob Herring <[email protected]>
> Signed-off-by: Horia Geantă <[email protected]>
Acked-by: Rob Herring <[email protected]>
On Mon, Apr 03, 2017 at 06:12:04PM +0300, Horia Geantă wrote:
> The way Job Ring platform devices are created and released does not
> allow for multiple create-release cycles.
>
> JR0 Platform device creation error
> JR0 Platform device creation error
> caam 2100000.caam: no queues configured, terminating
> caam: probe of 2100000.caam failed with error -12
>
> The reason is that platform devices are created for each job ring:
>
> for_each_available_child_of_node(nprop, np)
> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) {
> ctrlpriv->jrpdev[ring] =
> of_platform_device_create(np, NULL, dev);
>
> which sets OF_POPULATED on the device node, but then it cleans these up:
>
> /* Remove platform devices for JobRs */
> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) {
> if (ctrlpriv->jrpdev[ring])
> of_device_unregister(ctrlpriv->jrpdev[ring]);
> }
>
> which leaves OF_POPULATED set.
>
> Use of_platform_populate / of_platform_depopulate instead.
> This allows for a bit of driver clean-up, jrpdev is no longer needed.
>
> Logic changes a bit too:
> -exit in case of_platform_populate fails, since currently even QI backend
> depends on JR; true, we no longer support the case when "some" of the JR
> DT nodes are incorrect
> -when cleaning up, caam_remove() would also depopulate RTIC in case
> it would have been populated somewhere else - not the case for now
>
> Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring")
> Reported-by: Russell King <[email protected]>
> Suggested-by: Rob Herring <[email protected]>
> Signed-off-by: Horia Geantă <[email protected]>
> ---
> Not sending this directly to -stable, since it does not apply cleanly.
Patch applied. I forced it to apply on the crypto tree, then merged
it forward to cryptodev. Please check the end result and let me know
if it's wrong.
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 4/5/2017 5:14 PM, Herbert Xu wrote:
> On Mon, Apr 03, 2017 at 06:12:04PM +0300, Horia Geant? wrote:
>> The way Job Ring platform devices are created and released does not
>> allow for multiple create-release cycles.
>>
>> JR0 Platform device creation error
>> JR0 Platform device creation error
>> caam 2100000.caam: no queues configured, terminating
>> caam: probe of 2100000.caam failed with error -12
>>
>> The reason is that platform devices are created for each job ring:
>>
>> for_each_available_child_of_node(nprop, np)
>> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") ||
>> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) {
>> ctrlpriv->jrpdev[ring] =
>> of_platform_device_create(np, NULL, dev);
>>
>> which sets OF_POPULATED on the device node, but then it cleans these up:
>>
>> /* Remove platform devices for JobRs */
>> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) {
>> if (ctrlpriv->jrpdev[ring])
>> of_device_unregister(ctrlpriv->jrpdev[ring]);
>> }
>>
>> which leaves OF_POPULATED set.
>>
>> Use of_platform_populate / of_platform_depopulate instead.
>> This allows for a bit of driver clean-up, jrpdev is no longer needed.
>>
>> Logic changes a bit too:
>> -exit in case of_platform_populate fails, since currently even QI backend
>> depends on JR; true, we no longer support the case when "some" of the JR
>> DT nodes are incorrect
>> -when cleaning up, caam_remove() would also depopulate RTIC in case
>> it would have been populated somewhere else - not the case for now
>>
>> Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring")
>> Reported-by: Russell King <[email protected]>
>> Suggested-by: Rob Herring <[email protected]>
>> Signed-off-by: Horia Geant? <[email protected]>
>> ---
>> Not sending this directly to -stable, since it does not apply cleanly.
>
> Patch applied. I forced it to apply on the crypto tree, then merged
> it forward to cryptodev. Please check the end result and let me know
> if it's wrong.
>
Looks fine.
Thanks,
Horia