| Message ID | 20231011-mlx5_init_fix-v3-1-787ffb9183c6@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] net/mlx5: fix calling mlx5_cmd_init() before DMA mask is set | expand |
On 11 Oct 09:57, Niklas Schnelle wrote: >Since commit 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and >reload routines") mlx5_cmd_init() is called in mlx5_mdev_init() which is >called in probe_one() before mlx5_pci_init(). This is a problem because >mlx5_pci_init() is where the DMA and coherent mask is set but >mlx5_cmd_init() already does a dma_alloc_coherent(). Thus a DMA >allocation is done during probe before the correct mask is set. This >causes probe to fail initialization of the cmdif SW structs on s390x >after that is converted to the common dma-iommu code. This is because on >s390x DMA addresses below 4 GiB are reserved on current machines and >unlike the old s390x specific DMA API implementation common code >enforces DMA masks. > >Fix this by moving set_dma_caps() out of mlx5_pci_init() and into >probe_one() before mlx5_mdev_init(). To match the overall naming scheme >rename it to mlx5_dma_init(). How about we just call mlx5_pci_init() before mlx5_mdev_init(), instead of breaking it apart ? >
On 11 Oct 11:20, Saeed Mahameed wrote: >On 11 Oct 09:57, Niklas Schnelle wrote: >>Since commit 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and >>reload routines") mlx5_cmd_init() is called in mlx5_mdev_init() which is >>called in probe_one() before mlx5_pci_init(). This is a problem because >>mlx5_pci_init() is where the DMA and coherent mask is set but >>mlx5_cmd_init() already does a dma_alloc_coherent(). Thus a DMA >>allocation is done during probe before the correct mask is set. This >>causes probe to fail initialization of the cmdif SW structs on s390x >>after that is converted to the common dma-iommu code. This is because on >>s390x DMA addresses below 4 GiB are reserved on current machines and >>unlike the old s390x specific DMA API implementation common code >>enforces DMA masks. >> >>Fix this by moving set_dma_caps() out of mlx5_pci_init() and into >>probe_one() before mlx5_mdev_init(). To match the overall naming scheme >>rename it to mlx5_dma_init(). > >How about we just call mlx5_pci_init() before mlx5_mdev_init(), instead of >breaking it apart ? I just posted this RFC patch [1]: I am working in very limited conditions these days, and I don't have strong opinion on which approach to take, Leon, Niklas, please advise. The three possible solutions: 1) mlx5_pci_init() before mlx5_mdev_init(), I don't think enabling pci before initializing cmd dma would be a problem. 2) This patch. 3) Shay's patch from the link below: [1] https://patchwork.kernel.org/project/netdevbpf/patch/20231011184511.19818-1-saeed@kernel.org/ Thanks, Saeed.
On Wed, 2023-10-11 at 11:56 -0700, Saeed Mahameed wrote: > On 11 Oct 11:20, Saeed Mahameed wrote: > > On 11 Oct 09:57, Niklas Schnelle wrote: > > > Since commit 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and > > > reload routines") mlx5_cmd_init() is called in mlx5_mdev_init() which is > > > called in probe_one() before mlx5_pci_init(). This is a problem because > > > mlx5_pci_init() is where the DMA and coherent mask is set but > > > mlx5_cmd_init() already does a dma_alloc_coherent(). Thus a DMA > > > allocation is done during probe before the correct mask is set. This > > > causes probe to fail initialization of the cmdif SW structs on s390x > > > after that is converted to the common dma-iommu code. This is because on > > > s390x DMA addresses below 4 GiB are reserved on current machines and > > > unlike the old s390x specific DMA API implementation common code > > > enforces DMA masks. > > > > > > Fix this by moving set_dma_caps() out of mlx5_pci_init() and into > > > probe_one() before mlx5_mdev_init(). To match the overall naming scheme > > > rename it to mlx5_dma_init(). > > > > How about we just call mlx5_pci_init() before mlx5_mdev_init(), instead of > > breaking it apart ? > > I just posted this RFC patch [1]: This patch works to solve the problem as well. > > I am working in very limited conditions these days, and I don't have strong > opinion on which approach to take, Leon, Niklas, please advise. > > The three possible solutions: > > 1) mlx5_pci_init() before mlx5_mdev_init(), I don't think enabling pci > before initializing cmd dma would be a problem. > > 2) This patch. > > 3) Shay's patch from the link below: > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20231011184511.19818-1-saeed@kernel.org/ > > Thanks, > Saeed. My first gut feeling was option 1) but I'm just as happy with 2) or 3). For me option 2 is the least invasive but not by much. For me the important thing is what Jason also said yesterday. We need to merge something now to unbreak linux-next on s390x and to make sure we don't end up with a broken v6.7-rc1. This is already hampering our CI tests with linux-next. So let's do whatever can be merged the quickest and then feel free to do any refactoring ideas that this discussion might have spawned on top of that. My guess for this criteria would be 2). Thanks, Niklas
On Thu, 2023-10-12 at 12:53 +0200, Niklas Schnelle wrote: > On Wed, 2023-10-11 at 11:56 -0700, Saeed Mahameed wrote: > > On 11 Oct 11:20, Saeed Mahameed wrote: > > > On 11 Oct 09:57, Niklas Schnelle wrote: > > > > Since commit 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and > > > > reload routines") mlx5_cmd_init() is called in mlx5_mdev_init() which is > > > > called in probe_one() before mlx5_pci_init(). This is a problem because > > > > mlx5_pci_init() is where the DMA and coherent mask is set but > > > > mlx5_cmd_init() already does a dma_alloc_coherent(). Thus a DMA > > > > allocation is done during probe before the correct mask is set. This > > > > causes probe to fail initialization of the cmdif SW structs on s390x > > > > after that is converted to the common dma-iommu code. This is because on > > > > s390x DMA addresses below 4 GiB are reserved on current machines and > > > > unlike the old s390x specific DMA API implementation common code > > > > enforces DMA masks. > > > > > > > > Fix this by moving set_dma_caps() out of mlx5_pci_init() and into > > > > probe_one() before mlx5_mdev_init(). To match the overall naming scheme > > > > rename it to mlx5_dma_init(). > > > > > > How about we just call mlx5_pci_init() before mlx5_mdev_init(), instead of > > > breaking it apart ? > > > > I just posted this RFC patch [1]: > > This patch works to solve the problem as well. > > > > > I am working in very limited conditions these days, and I don't have strong > > opinion on which approach to take, Leon, Niklas, please advise. > > > > The three possible solutions: > > > > 1) mlx5_pci_init() before mlx5_mdev_init(), I don't think enabling pci > > before initializing cmd dma would be a problem. > > > > 2) This patch. > > > > 3) Shay's patch from the link below: > > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20231011184511.19818-1-saeed@kernel.org/ > > > > Thanks, > > Saeed. > > My first gut feeling was option 1) but I'm just as happy with 2) or 3). > For me option 2 is the least invasive but not by much. > > For me the important thing is what Jason also said yesterday. We need > to merge something now to unbreak linux-next on s390x and to make sure > we don't end up with a broken v6.7-rc1. This is already hampering our > CI tests with linux-next. So let's do whatever can be merged the > quickest and then feel free to do any refactoring ideas that this > discussion might have spawned on top of that. My guess for this > criteria would be 2). > > Thanks, > Niklas > Looking closer at the patch from Shay I do like that it changes the order in the disable/tear down path too. So since that also fixes a PPC issue I guess that may indeed be the best solution if we can get it merged quickly. I'll comment with my Tested-by there too. Thanks, Niklas
On 12 Oct 13:39, Niklas Schnelle wrote: >On Thu, 2023-10-12 at 12:53 +0200, Niklas Schnelle wrote: >> On Wed, 2023-10-11 at 11:56 -0700, Saeed Mahameed wrote: >> > On 11 Oct 11:20, Saeed Mahameed wrote: >> > > On 11 Oct 09:57, Niklas Schnelle wrote: >> > > > Since commit 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and >> > > > reload routines") mlx5_cmd_init() is called in mlx5_mdev_init() which is >> > > > called in probe_one() before mlx5_pci_init(). This is a problem because >> > > > mlx5_pci_init() is where the DMA and coherent mask is set but >> > > > mlx5_cmd_init() already does a dma_alloc_coherent(). Thus a DMA >> > > > allocation is done during probe before the correct mask is set. This >> > > > causes probe to fail initialization of the cmdif SW structs on s390x >> > > > after that is converted to the common dma-iommu code. This is because on >> > > > s390x DMA addresses below 4 GiB are reserved on current machines and >> > > > unlike the old s390x specific DMA API implementation common code >> > > > enforces DMA masks. >> > > > >> > > > Fix this by moving set_dma_caps() out of mlx5_pci_init() and into >> > > > probe_one() before mlx5_mdev_init(). To match the overall naming scheme >> > > > rename it to mlx5_dma_init(). >> > > >> > > How about we just call mlx5_pci_init() before mlx5_mdev_init(), instead of >> > > breaking it apart ? >> > >> > I just posted this RFC patch [1]: >> >> This patch works to solve the problem as well. >> >> > >> > I am working in very limited conditions these days, and I don't have strong >> > opinion on which approach to take, Leon, Niklas, please advise. >> > >> > The three possible solutions: >> > >> > 1) mlx5_pci_init() before mlx5_mdev_init(), I don't think enabling pci >> > before initializing cmd dma would be a problem. >> > >> > 2) This patch. >> > >> > 3) Shay's patch from the link below: >> > [1] https://patchwork.kernel.org/project/netdevbpf/patch/20231011184511.19818-1-saeed@kernel.org/ >> > >> > Thanks, >> > Saeed. >> >> My first gut feeling was option 1) but I'm just as happy with 2) or 3). >> For me option 2 is the least invasive but not by much. >> >> For me the important thing is what Jason also said yesterday. We need >> to merge something now to unbreak linux-next on s390x and to make sure >> we don't end up with a broken v6.7-rc1. This is already hampering our >> CI tests with linux-next. So let's do whatever can be merged the >> quickest and then feel free to do any refactoring ideas that this >> discussion might have spawned on top of that. My guess for this >> criteria would be 2). >> >> Thanks, >> Niklas >> > >Looking closer at the patch from Shay I do like that it changes the >order in the disable/tear down path too. So since that also fixes a PPC >issue I guess that may indeed be the best solution if we can get it >merged quickly. I'll comment with my Tested-by there too. > Ack, will take Shay's patch then, Will add your Test-by and Reviewed-by. >Thanks, >Niklas
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c index 15561965d2af..f251d233a16c 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c @@ -250,7 +250,7 @@ static void mlx5_set_driver_version(struct mlx5_core_dev *dev) mlx5_cmd_exec_in(dev, set_driver_version, in); } -static int set_dma_caps(struct pci_dev *pdev) +static int mlx5_dma_init(struct pci_dev *pdev) { int err; @@ -905,12 +905,6 @@ static int mlx5_pci_init(struct mlx5_core_dev *dev, struct pci_dev *pdev, pci_set_master(pdev); - err = set_dma_caps(pdev); - if (err) { - mlx5_core_err(dev, "Failed setting DMA capabilities mask, aborting\n"); - goto err_clr_master; - } - if (pci_enable_atomic_ops_to_root(pdev, PCI_EXP_DEVCAP2_ATOMIC_COMP32) && pci_enable_atomic_ops_to_root(pdev, PCI_EXP_DEVCAP2_ATOMIC_COMP64) && pci_enable_atomic_ops_to_root(pdev, PCI_EXP_DEVCAP2_ATOMIC_COMP128)) @@ -1908,9 +1902,15 @@ static int probe_one(struct pci_dev *pdev, const struct pci_device_id *id) goto adev_init_err; } + err = mlx5_dma_init(pdev); + if (err) { + mlx5_core_err(dev, "Failed setting DMA capabilities mask, aborting\n"); + goto dma_init_err; + } + err = mlx5_mdev_init(dev, prof_sel); if (err) - goto mdev_init_err; + goto dma_init_err; err = mlx5_pci_init(dev, pdev, id); if (err) { @@ -1942,7 +1942,7 @@ static int probe_one(struct pci_dev *pdev, const struct pci_device_id *id) mlx5_pci_close(dev); pci_init_err: mlx5_mdev_uninit(dev); -mdev_init_err: +dma_init_err: mlx5_adev_idx_free(dev->priv.adev_idx); adev_init_err: mlx5_devlink_free(devlink);
Since commit 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and reload routines") mlx5_cmd_init() is called in mlx5_mdev_init() which is called in probe_one() before mlx5_pci_init(). This is a problem because mlx5_pci_init() is where the DMA and coherent mask is set but mlx5_cmd_init() already does a dma_alloc_coherent(). Thus a DMA allocation is done during probe before the correct mask is set. This causes probe to fail initialization of the cmdif SW structs on s390x after that is converted to the common dma-iommu code. This is because on s390x DMA addresses below 4 GiB are reserved on current machines and unlike the old s390x specific DMA API implementation common code enforces DMA masks. Fix this by moving set_dma_caps() out of mlx5_pci_init() and into probe_one() before mlx5_mdev_init(). To match the overall naming scheme rename it to mlx5_dma_init(). Link: https://lore.kernel.org/linux-iommu/cfc9e9128ed5571d2e36421e347301057662a09e.camel@linux.ibm.com/ Fixes: 06cd555f73ca ("net/mlx5: split mlx5_cmd_init() to probe and reload routines") Reviewed-by: Leon Romanovsky <leonro@nvidia.com> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> --- Note: I ran into this while testing the linked series for converting s390x to use dma-iommu. The existing s390x specific DMA API implementation doesn't respect DMA masks and is thus not affected. --- Changes in v3: - Added R-b's from Leon R and Jacob K - Link to v2: https://lore.kernel.org/r/20230929-mlx5_init_fix-v2-1-51ed2094c9d8@linux.ibm.com --- drivers/net/ethernet/mellanox/mlx5/core/main.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) --- base-commit: 94f6f0550c625fab1f373bb86a6669b45e9748b3 change-id: 20230928-mlx5_init_fix-c465b5cda327 Best regards,