| Message ID | 20220426201539.12829-1-sven@svenpeter.dev |
|---|---|
| Headers | show |
| Series | Apple M1 (Pro/Max) NVMe driver | expand |
On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: > > The NVMe co-processor on the Apple M1 uses a DMA address filter called > SART for some DMA transactions. This adds a simple driver used to > configure the memory regions from which DMA transactions are allowed. > > Unlike a real IOMMU, SART does not support any pagetables and can't be > implemented inside the IOMMU subsystem using iommu_ops. > > It also can't be implemented using dma_map_ops since not all DMA > transactions of the NVMe controller are filtered by SART. > Instead, most buffers have to be registered using the integrated NVMe > IOMMU and we can't have two separate dma_map_ops implementations for a > single device. > > Co-developed-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Sven Peter <sven@svenpeter.dev> Reviewed-by: Arnd Bergmann <arnd@arndb.de> One more detail I noticed: > +#if IS_ENABLED(CONFIG_APPLE_SART) > + > +struct apple_sart; > + > +/* > + * Get a reference to the SART attached to dev. > + * > + * Looks for the phandle reference in apple,sart and returns a pointer > + * to the corresponding apple_sart struct to be used with > + * apple_sart_add_allowed_region and apple_sart_remove_allowed_region. > + */ > +struct apple_sart *devm_apple_sart_get(struct device *dev); > + In general, I don't like to hide declarations in a header behind an #if. Unless there is a good reason for doing this, just make the declaration unconditional, which avoids recompiling when the symbol changes. The only other effect is that you get a link-time error instead of a compile-time error if you messed up the Kconfig dependencies, but as long as the dependencies are correct it will be fine either way. Arnd
On Tue, Apr 26, 2022 at 10:15:39PM +0200, Sven Peter wrote: > +static enum blk_eh_timer_return apple_nvme_timeout(struct request *req, > + bool reserved) > +{ > + struct apple_nvme_iod *iod = blk_mq_rq_to_pdu(req); > + struct apple_nvme_queue *q = iod->q; > + struct apple_nvme *anv = queue_to_apple_nvme(q); > + unsigned long flags; > + u32 csts = readl(anv->mmio_nvme + NVME_REG_CSTS); > + > + if (anv->ctrl.state != NVME_CTRL_LIVE) { > + /* > + * From rdma.c: > + * If we are resetting, connecting or deleting we should > + * complete immediately because we may block controller > + * teardown or setup sequence > + * - ctrl disable/shutdown fabrics requests > + * - connect requests > + * - initialization admin requests > + * - I/O requests that entered after unquiescing and > + * the controller stopped responding > + * > + * All other requests should be cancelled by the error > + * recovery work, so it's fine that we fail it here. > + */ > + dev_warn(anv->dev, > + "I/O %d(aq:%d) timeout while not in live state\n", > + req->tag, q->is_adminq); > + if (blk_mq_request_started(req) && > + !blk_mq_request_completed(req)) { > + nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD; > + blk_mq_complete_request(req); I think you need a 'nvme_req(req)->flags |= NVME_REQ_CANCELLED' here to get the expected -EINTR for any admin command timeouts during a reset. Without it, the resetting task is going to think it got a real response from the controller. Other than that, this looks good.
On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: > > Apple SoCs such as the M1 come with multiple embedded co-processors > running proprietary firmware. Communication with those is established > over a simple mailbox using the RTKit IPC protocol. > > This cannot be implement inside the mailbox subsystem since on top > of communication over channels we also need support for starting, > hibernating and resetting these co-processors. We also need to > handle shared memory allocations differently depending on the > co-processor and don't want to split that across multiple drivers. > > Signed-off-by: Sven Peter <sven@svenpeter.dev> Reviewed-by: Arnd Bergmann <arnd@arndb.de> > +bool apple_rtkit_is_running(struct apple_rtkit *rtk) > +{ > + if (rtk->crashed) > + return false; > + if ((rtk->iop_power_state & 0xff) != APPLE_RTKIT_PWR_STATE_ON) > + return false; > + if ((rtk->ap_power_state & 0xff) != APPLE_RTKIT_PWR_STATE_ON) > + return false; > + return true; > +} > +EXPORT_SYMBOL_GPL(apple_rtkit_is_running); > + > +bool apple_rtkit_is_crashed(struct apple_rtkit *rtk) > +{ > + return rtk->crashed; > +} > +EXPORT_SYMBOL_GPL(apple_rtkit_is_crashed); I noticed that you use EXPORT_SYMBOL_GPL() here, but a more permissive EXPORT_SYMBOL() in the SART driver. Is that intentional? I would have assumed that both are similarly private to the apple SoCs and would be the _GPL type, but this is something you get to pick as the author. > + > +#if IS_ENABLED(CONFIG_APPLE_RTKIT) > + Same comment about the #if as for the SART driver: I'd prefer it without the conditional compilation. Arnd
On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: > > Apple SoCs such as the M1 come with an embedded NVMe controller that > is not attached to any PCIe bus. Additionally, it doesn't conform > to the NVMe specification and requires a bunch of changes to command > submission and IOMMU configuration to work. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Sven Peter <sven@svenpeter.dev> I did not do a detailed review of this again, but the previous version seemed mostly fine already. Reviewed-by: Arnd Bergmann <arnd@arndb.de> One suggestion for a very minor improvement: > +#ifdef CONFIG_PM_SLEEP > +static int apple_nvme_resume(struct device *dev) > +{ ... > +} > + > +static const struct dev_pm_ops apple_nvme_pm_ops = { > + .suspend = apple_nvme_suspend, > + .resume = apple_nvme_resume, > +}; > +#endif > + > +static const struct of_device_id apple_nvme_of_match[] = { > + { .compatible = "apple,nvme-ans2" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, apple_nvme_of_match); > + > +static struct platform_driver apple_nvme_driver = { > + .driver = { > + .name = "nvme-apple", > + .of_match_table = apple_nvme_of_match, > +#ifdef CONFIG_PM_SLEEP > + .pm = &apple_nvme_pm_ops, > +#endif > + }, You can now use "static DEFINE_SIMPLE_DEV_PM_OPS()" to define the pm operations without these #ifdefs. Arnd
On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: > > Hi, > > This series includes everything[*] required to get NVMe up and running on > Apple's M1, M1 Pro and M1 Max SoCs. > > v1: https://lore.kernel.org/linux-nvme/20220321165049.35985-1-sven@svenpeter.dev/T/ > v2: https://lore.kernel.org/linux-nvme/20220415142055.30873-1-sven@svenpeter.dev/T/ > > Thanks everyone for the reviews of v2 again! There are just some minor > changes since v2 listed in the individual commits again. Nice! I had not looked at v2 in much detail, but I'm perfectly happy with this version, I found a few things that could be improved if you do a respin, but nothing important. > [*] The only missing part in this series are the device tree updates > but since these will go through arm-soc anyway I haven't included > them here but will instead submit them once this series is in a shape > where it can be merged. Just as a clarification: the drivers/soc/ portion should normally go through the soc tree as well, but I'm happy for those to get merged along with the nvme driver if that helps get it all mainlined more quickly. Arnd
On Tue, Apr 26, 2022, at 23:01, Arnd Bergmann wrote: > On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: >> >> Apple SoCs such as the M1 come with multiple embedded co-processors >> running proprietary firmware. Communication with those is established >> over a simple mailbox using the RTKit IPC protocol. >> >> This cannot be implement inside the mailbox subsystem since on top >> of communication over channels we also need support for starting, >> hibernating and resetting these co-processors. We also need to >> handle shared memory allocations differently depending on the >> co-processor and don't want to split that across multiple drivers. >> >> Signed-off-by: Sven Peter <sven@svenpeter.dev> > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > thanks! >> +bool apple_rtkit_is_running(struct apple_rtkit *rtk) >> +{ >> + if (rtk->crashed) >> + return false; >> + if ((rtk->iop_power_state & 0xff) != APPLE_RTKIT_PWR_STATE_ON) >> + return false; >> + if ((rtk->ap_power_state & 0xff) != APPLE_RTKIT_PWR_STATE_ON) >> + return false; >> + return true; >> +} >> +EXPORT_SYMBOL_GPL(apple_rtkit_is_running); >> + >> +bool apple_rtkit_is_crashed(struct apple_rtkit *rtk) >> +{ >> + return rtk->crashed; >> +} >> +EXPORT_SYMBOL_GPL(apple_rtkit_is_crashed); > > I noticed that you use EXPORT_SYMBOL_GPL() here, but a more permissive > EXPORT_SYMBOL() in the SART driver. Is that intentional? No idea why I used both but it wasn't intentional. I'll change all exports to EXPORT_SYMBOL_GPL. > >> + >> +#if IS_ENABLED(CONFIG_APPLE_RTKIT) >> + > > Same comment about the #if as for the SART driver: I'd prefer it without the > conditional compilation. Ok, will remove those #ifs as well. Sven
On Tue, Apr 26, 2022, at 23:15, Arnd Bergmann wrote: > On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: >> >> Hi, >> >> This series includes everything[*] required to get NVMe up and running on >> Apple's M1, M1 Pro and M1 Max SoCs. >> >> v1: https://lore.kernel.org/linux-nvme/20220321165049.35985-1-sven@svenpeter.dev/T/ >> v2: https://lore.kernel.org/linux-nvme/20220415142055.30873-1-sven@svenpeter.dev/T/ >> >> Thanks everyone for the reviews of v2 again! There are just some minor >> changes since v2 listed in the individual commits again. > > Nice! I had not looked at v2 in much detail, but I'm perfectly happy > with this version, > > I found a few things that could be improved if you do a respin, but > nothing important. Thanks, I'll respin it later this week to fix those things! > >> [*] The only missing part in this series are the device tree updates >> but since these will go through arm-soc anyway I haven't included >> them here but will instead submit them once this series is in a shape >> where it can be merged. > > Just as a clarification: the drivers/soc/ portion should normally go through the > soc tree as well, but I'm happy for those to get merged along with the > nvme driver > if that helps get it all mainlined more quickly. Makes sense! I don't think I'll be ready to submit USB3/USB4/Thunderbolt (which also needs RTKit) during this cycle but I think there's a decent chance marcan will submit SMC which also depends on RTKit and will go through a different subsystem. What's the best way to handle the RTKit commits in that case? It would be great if we could get both into 5.19. Sven
On Tue, Apr 26, 2022, at 23:00, Keith Busch wrote: > On Tue, Apr 26, 2022 at 10:15:39PM +0200, Sven Peter wrote: >> +static enum blk_eh_timer_return apple_nvme_timeout(struct request *req, >> + bool reserved) >> +{ >> + struct apple_nvme_iod *iod = blk_mq_rq_to_pdu(req); >> + struct apple_nvme_queue *q = iod->q; >> + struct apple_nvme *anv = queue_to_apple_nvme(q); >> + unsigned long flags; >> + u32 csts = readl(anv->mmio_nvme + NVME_REG_CSTS); >> + >> + if (anv->ctrl.state != NVME_CTRL_LIVE) { >> + /* >> + * From rdma.c: >> + * If we are resetting, connecting or deleting we should >> + * complete immediately because we may block controller >> + * teardown or setup sequence >> + * - ctrl disable/shutdown fabrics requests >> + * - connect requests >> + * - initialization admin requests >> + * - I/O requests that entered after unquiescing and >> + * the controller stopped responding >> + * >> + * All other requests should be cancelled by the error >> + * recovery work, so it's fine that we fail it here. >> + */ >> + dev_warn(anv->dev, >> + "I/O %d(aq:%d) timeout while not in live state\n", >> + req->tag, q->is_adminq); >> + if (blk_mq_request_started(req) && >> + !blk_mq_request_completed(req)) { >> + nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD; >> + blk_mq_complete_request(req); > > I think you need a 'nvme_req(req)->flags |= NVME_REQ_CANCELLED' here to get the > expected -EINTR for any admin command timeouts during a reset. Without it, the > resetting task is going to think it got a real response from the controller. Makes sense, will add it. Sven
On Wed, Apr 27, 2022 at 5:33 PM Sven Peter <sven@svenpeter.dev> wrote: > On Tue, Apr 26, 2022, at 23:15, Arnd Bergmann wrote: > > On Tue, Apr 26, 2022 at 10:15 PM Sven Peter <sven@svenpeter.dev> wrote: > > Makes sense! > I don't think I'll be ready to submit USB3/USB4/Thunderbolt (which also needs > RTKit) during this cycle but I think there's a decent chance marcan will submit > SMC which also depends on RTKit and will go through a different subsystem. > What's the best way to handle the RTKit commits in that case? > It would be great if we could get both into 5.19. The usual trick is to have a branch with the shared patches and have that pulled into every other tree that needs these, but make sure you never rebase. In this case, you could have something like a) rtkit driver in a shared branch (private only) b) thunderbolt driver based on branch a), merged through thunderbolt/usb/pci tree (I don't know who is responsible here) c) sart driver based on branch a), merged through soc tree d) nvme driver based on branch c), merged through nvme tree since the commit hashes are all identical, each patch only shows up in the git tree once, but you get a somewhat funny history. Arnd
On Wed, Apr 27, 2022 at 07:39:49PM +0200, Arnd Bergmann wrote: > The usual trick is to have a branch with the shared patches and have > that pulled into every other tree that needs these, but make sure you never > rebase. In this case, you could have something like > > a) rtkit driver in a shared branch (private only) > b) thunderbolt driver based on branch a), merged through > thunderbolt/usb/pci tree (I don't know who is responsible here) > c) sart driver based on branch a), merged through soc tree > d) nvme driver based on branch c), merged through nvme tree > > since the commit hashes are all identical, each patch only shows up in > the git tree once, but you get a somewhat funny history. Given that the nvme driver is just addition of new code I'm perfectly fine with sending it through whatever tree is most convenient.
On Thu, Apr 28, 2022, at 16:24, hch@lst.de wrote: > On Wed, Apr 27, 2022 at 07:39:49PM +0200, Arnd Bergmann wrote: >> The usual trick is to have a branch with the shared patches and have >> that pulled into every other tree that needs these, but make sure you never >> rebase. In this case, you could have something like >> >> a) rtkit driver in a shared branch (private only) >> b) thunderbolt driver based on branch a), merged through >> thunderbolt/usb/pci tree (I don't know who is responsible here) >> c) sart driver based on branch a), merged through soc tree >> d) nvme driver based on branch c), merged through nvme tree >> >> since the commit hashes are all identical, each patch only shows up in >> the git tree once, but you get a somewhat funny history. > > Given that the nvme driver is just addition of new code I'm perfectly > fine with sending it through whatever tree is most convenient. So If I understand all this correctly either 1) I send a pull request with rtkit+sart to Arnd/soc@ followed by a pull request with the same commits + the nvme driver to Christoph/nvme to make sure the commit hashes in both trees are the same. or 2) I send a pull request with rtkit+sart+nvme to soc@ and we take the entire driver through there with Christoph's ack if Arnd is fine with carrying it as well. In either case SMC (or thunderbolt if I finish in time) can also be based on the same rtkit commit and could go into 5.19 as well. I don't have any preference here (not that my opinion matters much for this decision anyway :-)) Sven
On Fri, Apr 29, 2022 at 6:37 PM Sven Peter <sven@svenpeter.dev> wrote: > On Thu, Apr 28, 2022, at 16:24, hch@lst.de wrote: > > On Wed, Apr 27, 2022 at 07:39:49PM +0200, Arnd Bergmann wrote: > >> The usual trick is to have a branch with the shared patches and have > >> that pulled into every other tree that needs these, but make sure you never > >> rebase. In this case, you could have something like > >> > >> a) rtkit driver in a shared branch (private only) > >> b) thunderbolt driver based on branch a), merged through > >> thunderbolt/usb/pci tree (I don't know who is responsible here) > >> c) sart driver based on branch a), merged through soc tree > >> d) nvme driver based on branch c), merged through nvme tree > >> > >> since the commit hashes are all identical, each patch only shows up in > >> the git tree once, but you get a somewhat funny history. > > > > Given that the nvme driver is just addition of new code I'm perfectly > > fine with sending it through whatever tree is most convenient. > > So If I understand all this correctly either > 1) I send a pull request with rtkit+sart to Arnd/soc@ followed by > a pull request with the same commits + the nvme driver to > Christoph/nvme to make sure the commit hashes in both trees > are the same. > or > 2) I send a pull request with rtkit+sart+nvme to soc@ and we > take the entire driver through there with Christoph's ack > if Arnd is fine with carrying it as well. > > In either case SMC (or thunderbolt if I finish in time) can also be based > on the same rtkit commit and could go into 5.19 as well. > I don't have any preference here (not that my opinion matters much > for this decision anyway :-)) Correct, those are both ok. Arnd
Hi, This series includes everything[*] required to get NVMe up and running on Apple's M1, M1 Pro and M1 Max SoCs. v1: https://lore.kernel.org/linux-nvme/20220321165049.35985-1-sven@svenpeter.dev/T/ v2: https://lore.kernel.org/linux-nvme/20220415142055.30873-1-sven@svenpeter.dev/T/ Thanks everyone for the reviews of v2 again! There are just some minor changes since v2 listed in the individual commits again. Thanks, Sven [*] The only missing part in this series are the device tree updates but since these will go through arm-soc anyway I haven't included them here but will instead submit them once this series is in a shape where it can be merged. Sven Peter (6): dt-bindings: iommu: Add Apple SART DMA address filter dt-bindings: nvme: Add Apple ANS NVMe soc: apple: Always include Makefile soc: apple: Add SART driver soc: apple: Add RTKit IPC library nvme-apple: Add initial Apple SoC NVMe driver .../devicetree/bindings/iommu/apple,sart.yaml | 52 + .../bindings/nvme/apple,nvme-ans.yaml | 111 ++ MAINTAINERS | 4 + drivers/nvme/host/Kconfig | 13 + drivers/nvme/host/Makefile | 3 + drivers/nvme/host/apple.c | 1598 +++++++++++++++++ drivers/soc/Makefile | 2 +- drivers/soc/apple/Kconfig | 24 + drivers/soc/apple/Makefile | 6 + drivers/soc/apple/rtkit-crashlog.c | 154 ++ drivers/soc/apple/rtkit-internal.h | 62 + drivers/soc/apple/rtkit.c | 958 ++++++++++ drivers/soc/apple/sart.c | 328 ++++ include/linux/soc/apple/rtkit.h | 159 ++ include/linux/soc/apple/sart.h | 57 + 15 files changed, 3530 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/iommu/apple,sart.yaml create mode 100644 Documentation/devicetree/bindings/nvme/apple,nvme-ans.yaml create mode 100644 drivers/nvme/host/apple.c create mode 100644 drivers/soc/apple/rtkit-crashlog.c create mode 100644 drivers/soc/apple/rtkit-internal.h create mode 100644 drivers/soc/apple/rtkit.c create mode 100644 drivers/soc/apple/sart.c create mode 100644 include/linux/soc/apple/rtkit.h create mode 100644 include/linux/soc/apple/sart.h