Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On 08/11/2019 17:48, Will Deacon wrote: On Fri, Nov 08, 2019 at 05:32:48PM +, Will Deacon wrote: On Fri, Nov 08, 2019 at 05:25:09PM +, John Garry wrote: On 08/11/2019 16:47, Will Deacon wrote: On Fri, Nov 08, 2019 at 04:44:25PM +, John Garry wrote: BTW, it now looks like it was your v1 series I was testing there, on your branch iommu/module. It would be helpful to update for ease of testing. Yes, sorry about that. I'll update it now (although I'm not sure it will help with this -- I was going to see what happens with other devices such as the intel-iommu or storage controllers) So I tried your v2 series for this - it has the same issue, as I anticipated. Right, I'm just not sure how resilient drivers are expected to be to force unbinding like this. You can break lots of stuff with root... It seems that some iommu drivers do call iommu_device_register(), so maybe a decent reference. Or simply stop the driver being unbound. I'm not sure what you mean about iommu_device_register() (we call that already), but I guess we can keep the '.suppress_bind_attrs = true' if necessary. I'll have a play on my laptop and see how well that works if you start unbinding stuff. So unbinding the nvme driver goes bang: [90139.090158] nvme nvme0: failed to set APST feature (-19) [90141.966780] Aborting journal on device dm-1-8. [90141.967124] Buffer I/O error on dev dm-1, logical block 26247168, lost sync page write [90141.967169] JBD2: Error -5 detected when updating journal superblock for dm-1-8. [90141.967403] Buffer I/O error on dev dm-1, logical block 0, lost sync page write [90141.967454] EXT4-fs (dm-1): I/O error while writing superblock [90141.967467] EXT4-fs error (device dm-1): ext4_journal_check_start:61: Detected aborted journal [90141.967473] EXT4-fs (dm-1): Remounting filesystem read-only [90141.967569] Buffer I/O error on dev dm-1, logical block 0, lost sync page write [90141.967682] EXT4-fs (dm-1): I/O error while writing superblock and I've not managed to recover the thing yet (it's stuck trying to reboot.) Not surprised. I guess the device backing your root directory disappeared. What state was your system in after unbinding the SMMU? Unusable again. For me the storage controller backing the root directory is compromised by disabling the SMMU unsafely. Thanks, John ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On 08/11/2019 17:32, Will Deacon wrote: On Fri, Nov 08, 2019 at 05:25:09PM +, John Garry wrote: On 08/11/2019 16:47, Will Deacon wrote: On Fri, Nov 08, 2019 at 04:44:25PM +, John Garry wrote: BTW, it now looks like it was your v1 series I was testing there, on your branch iommu/module. It would be helpful to update for ease of testing. Yes, sorry about that. I'll update it now (although I'm not sure it will help with this -- I was going to see what happens with other devices such as the intel-iommu or storage controllers) So I tried your v2 series for this - it has the same issue, as I anticipated. Right, I'm just not sure how resilient drivers are expected to be to force unbinding like this. You can break lots of stuff with root... For sure, but it is good practice to limit that. I had to fix something like this recently, so know about it... another potential problem is use-after-frees, where your device managed memory is freed at removal but still registered somewhere. It seems that some iommu drivers do call iommu_device_register(), so maybe a decent reference. Or simply stop the driver being unbound. I'm not sure what you mean about iommu_device_register() (we call that already), Sorry, I meant to say iommu_device_unregister(). but I guess we can keep the '.suppress_bind_attrs = true' if necessary. It may be good to add it to older stable kernels also, pre c07b6426df92. I'll have a play on my laptop and see how well that works if you start unbinding stuff. Cheers, John ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On Fri, Nov 08, 2019 at 05:32:48PM +, Will Deacon wrote: > On Fri, Nov 08, 2019 at 05:25:09PM +, John Garry wrote: > > On 08/11/2019 16:47, Will Deacon wrote: > > > On Fri, Nov 08, 2019 at 04:44:25PM +, John Garry wrote: > > > > BTW, it now looks like it was your v1 series I was testing there, on > > > > your > > > > branch iommu/module. It would be helpful to update for ease of testing. > > > > > > Yes, sorry about that. I'll update it now (although I'm not sure it will > > > help with this -- I was going to see what happens with other devices such > > > as the intel-iommu or storage controllers) > > > > So I tried your v2 series for this - it has the same issue, as I > > anticipated. > > Right, I'm just not sure how resilient drivers are expected to be to force > unbinding like this. You can break lots of stuff with root... > > > It seems that some iommu drivers do call iommu_device_register(), so maybe a > > decent reference. Or simply stop the driver being unbound. > > I'm not sure what you mean about iommu_device_register() (we call that > already), but I guess we can keep the '.suppress_bind_attrs = true' if > necessary. I'll have a play on my laptop and see how well that works if > you start unbinding stuff. So unbinding the nvme driver goes bang: [90139.090158] nvme nvme0: failed to set APST feature (-19) [90141.966780] Aborting journal on device dm-1-8. [90141.967124] Buffer I/O error on dev dm-1, logical block 26247168, lost sync page write [90141.967169] JBD2: Error -5 detected when updating journal superblock for dm-1-8. [90141.967403] Buffer I/O error on dev dm-1, logical block 0, lost sync page write [90141.967454] EXT4-fs (dm-1): I/O error while writing superblock [90141.967467] EXT4-fs error (device dm-1): ext4_journal_check_start:61: Detected aborted journal [90141.967473] EXT4-fs (dm-1): Remounting filesystem read-only [90141.967569] Buffer I/O error on dev dm-1, logical block 0, lost sync page write [90141.967682] EXT4-fs (dm-1): I/O error while writing superblock and I've not managed to recover the thing yet (it's stuck trying to reboot.) What state was your system in after unbinding the SMMU? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On Fri, Nov 08, 2019 at 05:25:09PM +, John Garry wrote: > On 08/11/2019 16:47, Will Deacon wrote: > > On Fri, Nov 08, 2019 at 04:44:25PM +, John Garry wrote: > > > BTW, it now looks like it was your v1 series I was testing there, on your > > > branch iommu/module. It would be helpful to update for ease of testing. > > > > Yes, sorry about that. I'll update it now (although I'm not sure it will > > help with this -- I was going to see what happens with other devices such > > as the intel-iommu or storage controllers) > > So I tried your v2 series for this - it has the same issue, as I > anticipated. Right, I'm just not sure how resilient drivers are expected to be to force unbinding like this. You can break lots of stuff with root... > It seems that some iommu drivers do call iommu_device_register(), so maybe a > decent reference. Or simply stop the driver being unbound. I'm not sure what you mean about iommu_device_register() (we call that already), but I guess we can keep the '.suppress_bind_attrs = true' if necessary. I'll have a play on my laptop and see how well that works if you start unbinding stuff. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On 08/11/2019 16:47, Will Deacon wrote:
On Fri, Nov 08, 2019 at 04:44:25PM +, John Garry wrote:
On 08/11/2019 16:17, John Garry wrote:
On 08/11/2019 15:16, Will Deacon wrote:
+MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
Hi Will,
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu-v3",
.of_match_table = of_match_ptr(arm_smmu_of_match),
- .suppress_bind_attrs = true,
Does this mean that we can now manually unbind this driver from the SMMU
device?
Seems dangerous. Here's what happens for me:
root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo
arm-smmu-v3.0.auto > unbind
[ 77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
ho [ 78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at
0x0146 [hwprod 0x0146, hwcons 0x]
},
.probe = arm_smmu_device_probe,
+ .remove = arm_smmu_device_remove,
.shutdown = arm_smmu_device_shutdown,
};
-builtin_platform_driver(arm_smmu_driver);
+module_platform_driver(arm_smmu_driver);
+
BTW, it now looks like it was your v1 series I was testing there, on your
branch iommu/module. It would be helpful to update for ease of testing.
Hi Will,
Yes, sorry about that. I'll update it now (although I'm not sure it will
help with this -- I was going to see what happens with other devices such
as the intel-iommu or storage controllers)
So I tried your v2 series for this - it has the same issue, as I
anticipated.
It seems that some iommu drivers do call iommu_device_register(), so
maybe a decent reference. Or simply stop the driver being unbound.
Cheers,
John
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On Fri, Nov 08, 2019 at 04:44:25PM +, John Garry wrote:
> On 08/11/2019 16:17, John Garry wrote:
> > On 08/11/2019 15:16, Will Deacon wrote:
> > > +MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
> >
> > Hi Will,
> >
> > > static struct platform_driver arm_smmu_driver = {
> > > .driver = {
> > > .name = "arm-smmu-v3",
> > > .of_match_table = of_match_ptr(arm_smmu_of_match),
> > > - .suppress_bind_attrs = true,
> >
> > Does this mean that we can now manually unbind this driver from the SMMU
> > device?
> >
> > Seems dangerous. Here's what happens for me:
> >
> > root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
> > ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo
> > arm-smmu-v3.0.auto > unbind
> > [ 77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
> > ho [ 78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at
> > 0x0146 [hwprod 0x0146, hwcons 0x]
> >
> > > },
> > > .probe = arm_smmu_device_probe,
> > > + .remove = arm_smmu_device_remove,
> > > .shutdown = arm_smmu_device_shutdown,
> > > };
> > > -builtin_platform_driver(arm_smmu_driver);
> > > +module_platform_driver(arm_smmu_driver);
> > > +
>
> BTW, it now looks like it was your v1 series I was testing there, on your
> branch iommu/module. It would be helpful to update for ease of testing.
Yes, sorry about that. I'll update it now (although I'm not sure it will
help with this -- I was going to see what happens with other devices such
as the intel-iommu or storage controllers)
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On 08/11/2019 16:17, John Garry wrote:
On 08/11/2019 15:16, Will Deacon wrote:
+MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
Hi Will,
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu-v3",
.of_match_table = of_match_ptr(arm_smmu_of_match),
- .suppress_bind_attrs = true,
Does this mean that we can now manually unbind this driver from the SMMU
device?
Seems dangerous. Here's what happens for me:
root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo
arm-smmu-v3.0.auto > unbind
[ 77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
ho [ 78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at
0x0146 [hwprod 0x0146, hwcons 0x]
},
.probe = arm_smmu_device_probe,
+ .remove = arm_smmu_device_remove,
.shutdown = arm_smmu_device_shutdown,
};
-builtin_platform_driver(arm_smmu_driver);
+module_platform_driver(arm_smmu_driver);
+
BTW, it now looks like it was your v1 series I was testing there, on
your branch iommu/module. It would be helpful to update for ease of testing.
Thanks,
John
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
On 08/11/2019 15:16, Will Deacon wrote:
+MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
Hi Will,
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu-v3",
.of_match_table = of_match_ptr(arm_smmu_of_match),
- .suppress_bind_attrs = true,
Does this mean that we can now manually unbind this driver from the SMMU
device?
Seems dangerous. Here's what happens for me:
root@ubuntu:/sys# cd ./bus/platform/drivers/arm-smmu-v3
ind @ubuntu:/sys/bus/platform/drivers/arm-smmu-v3# echo
arm-smmu-v3.0.auto > unbind
[ 77.580351] hisi_sas_v2_hw HISI0162:01: CQE_AXI_W_ERR (0x800) found!
ho [ 78.635473] platform arm-smmu-v3.0.auto: CMD_SYNC timeout at
0x0146 [hwprod 0x0146, hwcons 0x]
},
.probe = arm_smmu_device_probe,
+ .remove = arm_smmu_device_remove,
.shutdown = arm_smmu_device_shutdown,
};
-builtin_platform_driver(arm_smmu_driver);
+module_platform_driver(arm_smmu_driver);
+
Thanks,
John
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 6/9] Revert "iommu/arm-smmu: Make arm-smmu-v3 explicitly non-modular"
This reverts commit c07b6426df922d21a13a959cf785d46e9c531941.
Let's get the SMMUv3 driver building as a module, which means putting
back some dead code that we used to carry.
Signed-off-by: Will Deacon
---
drivers/iommu/arm-smmu-v3.c | 25 -
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 8da93e730d6f..2ad8e2ca0583 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -21,8 +21,7 @@
#include
#include
#include
-#include
-#include
+#include
#include
#include
#include
@@ -384,10 +383,6 @@
#define MSI_IOVA_BASE 0x800
#define MSI_IOVA_LENGTH0x10
-/*
- * not really modular, but the easiest way to keep compat with existing
- * bootargs behaviour is to continue using module_param_named here.
- */
static bool disable_bypass = 1;
module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO);
MODULE_PARM_DESC(disable_bypass,
@@ -3683,25 +3678,37 @@ static int arm_smmu_device_probe(struct platform_device
*pdev)
return 0;
}
-static void arm_smmu_device_shutdown(struct platform_device *pdev)
+static int arm_smmu_device_remove(struct platform_device *pdev)
{
struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
arm_smmu_device_disable(smmu);
+
+ return 0;
+}
+
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+ arm_smmu_device_remove(pdev);
}
static const struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v3", },
{ },
};
+MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu-v3",
.of_match_table = of_match_ptr(arm_smmu_of_match),
- .suppress_bind_attrs = true,
},
.probe = arm_smmu_device_probe,
+ .remove = arm_smmu_device_remove,
.shutdown = arm_smmu_device_shutdown,
};
-builtin_platform_driver(arm_smmu_driver);
+module_platform_driver(arm_smmu_driver);
+
+MODULE_DESCRIPTION("IOMMU API for ARM architected SMMUv3 implementations");
+MODULE_AUTHOR("Will Deacon ");
+MODULE_LICENSE("GPL v2");
--
2.24.0.rc1.363.gb1bccd3e3d-goog
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
