| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4d82c447-f957-5595-e5d2-f8a0dd629921@ti.com>
Date: Mon, 7 Nov 2022 15:23:08 +0200
From: Georgi Vlaev <g-vlaev@...com>
To: Péter Ujfalusi <peter.ujfalusi@...il.com>,
Vinod Koul <vkoul@...nel.org>
CC: Nishanth Menon <nm@...com>, <dmaengine@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
Vignesh Raghavendra <vigneshr@...com>,
Roger Quadros <rogerq@...nel.org>
Subject: Re: [PATCH v2] dmaengine: k3-udma: Add system suspend/resume support
Hi Peter,
On 11/5/22 20:49, Péter Ujfalusi wrote:
>
>
> On 03/11/2022 22:30, Georgi Vlaev wrote:
>> From: Vignesh Raghavendra <vigneshr@...com>
>>
>> The K3 platforms configure the DMA resources with the
>> help of the TI's System Firmware's Device Manager(DM)
>> over TISCI. The group of DMA related Resource Manager[1]
>> TISCI messages includes: INTA, RINGACC, UDMAP, and PSI-L.
>> This configuration however, does not persist in the DM
>> after leaving from Suspend-to-RAM state. We have to restore
>> the DMA channel configuration over TISCI for all configured
>> channels when returning from suspend.
>>
>> The TISCI resource management calls for each DMA type (UDMA,
>> PKTDMA, BCDMA) happen in device_free_chan_resources() and
>> device_alloc_chan_resources(). In pm_suspend() we store
>> the current udma_chan_config for channels that still have
>> attached clients and call device_free_chan_resources().
>> In pm_resume() restore the udma_channel_config from backup
>> and call device_alloc_chan_resources() for those channels.
>> Drivers like CPSW can do their own DMA resource management,
>> so use the late system suspend/resume hooks.
>
> It is wrong to push the DMA context store/restore task to a client driver (cpsw or icss), it has to be done by the glue layer.
>
> With this patch the DMAengine side of the UDMA/BCDMA/PKTDMA will support suspend/resume while the networking will remain broken, right?
The CPSW suspend/resume patch [0] releases the DMA resources in
suspend() and this one follows up in suspend_late() to deal with
what's left. The order is reversed when we resume back from suspend.
>
> I can not test this atm since my setup relies solely on NFS rootfs via cpsw, I might be able to check with a USB-ethernet dongle..
>
In this case you'll probably need CPSW suspend/resume patches [0]
and apply this one on top of that sequence.
> Please do a followup for the glue layer support.
>
Okay, will do. This may have some effect on the cpsw sequence though.
> Acked-by: Peter Ujfalusi <peter.ujfalusi@...il.com>
>
Thanks.
[0] https://lore.kernel.org/netdev/20221104132310.31577-1-rogerq@kernel.org/
>> [1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/index.html#resource-management-rm
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
>> [g-vlaev@...com: Add patch description and config backup]
>> [g-vlaev@...com: Supend only channels with clients]
>> Signed-off-by: Georgi Vlaev <g-vlaev@...com>
>> ---
>> Changes:
>>
>> v2:
>> * Update the commit message
>> * Use list_for_each_entry() to iterate over the channel list.
>>
>> drivers/dma/ti/k3-udma.c | 54 ++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 54 insertions(+)
>>
>> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
>> index ce8b80bb34d7..29844044132c 100644
>> --- a/drivers/dma/ti/k3-udma.c
>> +++ b/drivers/dma/ti/k3-udma.c
>> @@ -304,6 +304,8 @@ struct udma_chan {
>> /* Channel configuration parameters */
>> struct udma_chan_config config;
>> + /* Channel configuration parameters (backup) */
>> + struct udma_chan_config backup_config;
>> /* dmapool for packet mode descriptors */
>> bool use_dma_pool;
>> @@ -5491,11 +5493,63 @@ static int udma_probe(struct platform_device *pdev)
>> return ret;
>> }
>> +static int udma_pm_suspend(struct device *dev)
>> +{
>> + struct udma_dev *ud = dev_get_drvdata(dev);
>> + struct dma_device *dma_dev = &ud->ddev;
>> + struct dma_chan *chan;
>> + struct udma_chan *uc;
>> +
>> + list_for_each_entry(chan, &dma_dev->channels, device_node) {
>> + if (chan->client_count) {
>> + uc = to_udma_chan(chan);
>> + /* backup the channel configuration */
>> + memcpy(&uc->backup_config, &uc->config,
>> + sizeof(struct udma_chan_config));
>> + dev_dbg(dev, "Suspending channel %s\n",
>> + dma_chan_name(chan));
>> + ud->ddev.device_free_chan_resources(chan);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int udma_pm_resume(struct device *dev)
>> +{
>> + struct udma_dev *ud = dev_get_drvdata(dev);
>> + struct dma_device *dma_dev = &ud->ddev;
>> + struct dma_chan *chan;
>> + struct udma_chan *uc;
>> + int ret;
>> +
>> + list_for_each_entry(chan, &dma_dev->channels, device_node) {
>> + if (chan->client_count) {
>> + uc = to_udma_chan(chan);
>> + /* restore the channel configuration */
>> + memcpy(&uc->config, &uc->backup_config,
>> + sizeof(struct udma_chan_config));
>> + dev_dbg(dev, "Resuming channel %s\n",
>> + dma_chan_name(chan));
>> + ret = ud->ddev.device_alloc_chan_resources(chan);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct dev_pm_ops udma_pm_ops = {
>> + SET_LATE_SYSTEM_SLEEP_PM_OPS(udma_pm_suspend, udma_pm_resume)
>> +};
>> +
>> static struct platform_driver udma_driver = {
>> .driver = {
>> .name = "ti-udma",
>> .of_match_table = udma_of_match,
>> .suppress_bind_attrs = true,
>> + .pm = &udma_pm_ops,
>> },
>> .probe = udma_probe,
>> };
>>
>> base-commit: 81214a573d19ae2fa5b528286ba23cd1cb17feec
>
--
Regards,
Georgi
Powered by blists - more mailing lists