LinuxLists.cc - [PATCH v9 0/3] Add support for AMD PTDMA controller driver

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 02-06-21, 12:22, Sanjay R Mehta wrote:

> +static int pt_core_execute_cmd(struct ptdma_desc *desc, struct pt_cmd_queue *cmd_q)
> +{
> + bool soc = FIELD_GET(DWORD0_SOC, desc->dw0);
> + u8 *q_desc = (u8 *)&cmd_q->qbase[cmd_q->qidx];
> + u8 *dp = (u8 *)desc;

this case seems unnecessary?

> +int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
> + struct pt_passthru_engine *pt_engine)

Pls align this to preceding open brace, checkpatch with --strict would
warn you about this

> +static irqreturn_t pt_core_irq_handler(int irq, void *data)
> +{
> + struct pt_device *pt = data;
> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
> + u32 status;
> +
> + pt_core_disable_queue_interrupts(pt);
> +
> + status = ioread32(cmd_q->reg_interrupt_status);
> + if (status) {
> + cmd_q->int_status = status;
> + cmd_q->q_status = ioread32(cmd_q->reg_status);
> + cmd_q->q_int_status = ioread32(cmd_q->reg_int_status);
> +
> + /* On error, only save the first error value */
> + if ((status & INT_ERROR) && !cmd_q->cmd_error)
> + cmd_q->cmd_error = CMD_Q_ERROR(cmd_q->q_status);
> +
> + /* Acknowledge the interrupt */
> + iowrite32(status, cmd_q->reg_interrupt_status);
> + }
> +
> + pt_core_enable_queue_interrupts(pt);
> +
> + return IRQ_HANDLED;

should you always return IRQ_HANDLED, that sounds apt for the if loop
but not for the non loop case

> +int pt_core_init(struct pt_device *pt)
> +{
> + char dma_pool_name[MAX_DMAPOOL_NAME_LEN];
> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
> + u32 dma_addr_lo, dma_addr_hi;
> + struct device *dev = pt->dev;
> + struct dma_pool *dma_pool;
> + int ret;
> +
> + /* Allocate a dma pool for the queue */
> + snprintf(dma_pool_name, sizeof(dma_pool_name), "%s_q", pt->name);
> +
> + dma_pool = dma_pool_create(dma_pool_name, dev,
> + PT_DMAPOOL_MAX_SIZE,
> + PT_DMAPOOL_ALIGN, 0);
> + if (!dma_pool) {
> + dev_err(dev, "unable to allocate dma pool\n");

This is superfluous, allocator would warn on failure

> +static struct pt_device *pt_alloc_struct(struct device *dev)
> +{
> + struct pt_device *pt;
> +
> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> +
> + if (!pt)
> + return NULL;
> + pt->dev = dev;
> + pt->ord = atomic_inc_return(&pt_ordinal);

What is the use of this number?

> +static int pt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct pt_device *pt;
> + struct pt_msix *pt_msix;
> + struct device *dev = &pdev->dev;
> + void __iomem * const *iomap_table;
> + int bar_mask;
> + int ret = -ENOMEM;
> +
> + pt = pt_alloc_struct(dev);
> + if (!pt)
> + goto e_err;
> +
> + pt_msix = devm_kzalloc(dev, sizeof(*pt_msix), GFP_KERNEL);
> + if (!pt_msix)
> + goto e_err;
> +
> + pt->pt_msix = pt_msix;
> + pt->dev_vdata = (struct pt_dev_vdata *)id->driver_data;
> + if (!pt->dev_vdata) {
> + ret = -ENODEV;
> + dev_err(dev, "missing driver data\n");
> + goto e_err;
> + }
> +
> + ret = pcim_enable_device(pdev);
> + if (ret) {
> + dev_err(dev, "pcim_enable_device failed (%d)\n", ret);
> + goto e_err;
> + }
> +
> + bar_mask = pci_select_bars(pdev, IORESOURCE_MEM);
> + ret = pcim_iomap_regions(pdev, bar_mask, "ptdma");
> + if (ret) {
> + dev_err(dev, "pcim_iomap_regions failed (%d)\n", ret);
> + goto e_err;
> + }
> +
> + iomap_table = pcim_iomap_table(pdev);
> + if (!iomap_table) {
> + dev_err(dev, "pcim_iomap_table failed\n");
> + ret = -ENOMEM;
> + goto e_err;
> + }
> +
> + pt->io_regs = iomap_table[pt->dev_vdata->bar];
> + if (!pt->io_regs) {
> + dev_err(dev, "ioremap failed\n");
> + ret = -ENOMEM;
> + goto e_err;
> + }
> +
> + ret = pt_get_irqs(pt);
> + if (ret)
> + goto e_err;
> +
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
> + if (ret) {
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n",
> + ret);
> + goto e_err;
> + }
> + }
> +
> + dev_set_drvdata(dev, pt);
> +
> + if (pt->dev_vdata)
> + ret = pt_core_init(pt);
> +
> + if (ret)
> + goto e_err;
> +
> + dev_dbg(dev, "PTDMA enabled\n");

pls remove these...

> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +#include <linux/dmapool.h>
> +
> +#define MAX_PT_NAME_LEN 16
> +#define MAX_DMAPOOL_NAME_LEN 32
> +
> +#define MAX_HW_QUEUES 1
> +#define MAX_CMD_QLEN 100
> +
> +#define PT_ENGINE_PASSTHRU 5
> +#define PT_OFFSET 0x0
> +
> +#define PT_VSIZE 16
> +#define PT_VMASK ((unsigned int)((1 << PT_VSIZE) - 1))

why cast?

--
~Vinod

2021-06-09 15:35:27

[permalink] [raw]

Subject: Re: [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA

On 02-06-21, 12:22, Sanjay R Mehta wrote:

> +/* DebugFS helpers */
> +#define MAX_NAME_LEN 20
> +#define RI_VERSION_NUM 0x0000003F
> +
> +#define RI_NUM_VQM 0x00078000
> +#define RI_NVQM_SHIFT 15
> +
> +static DEFINE_MUTEX(pt_debugfs_lock);

unused?

> +
> +static int pt_debugfs_info_show(struct seq_file *s, void *p)
> +{
> + struct pt_device *pt = s->private;
> + unsigned int regval;
> +
> + if (!pt)
> + return 0;

better return an error code?

> +
> + seq_printf(s, "Device name: %s\n", pt->name);
> + seq_printf(s, " # Queues: %d\n", 1);
> + seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
> +
> + regval = ioread32(pt->io_regs + CMD_PT_VERSION);

how do you ensure your device is not sleeping or you can access iomem
safely?

> +void ptdma_debugfs_setup(struct pt_device *pt)
> +{
> + struct pt_cmd_queue *cmd_q;
> + char name[MAX_NAME_LEN + 1];
> + struct dentry *debugfs_q_instance;
> +
> + if (!debugfs_initialized())
> + return;
> +
> + debugfs_create_file("info", 0400, pt->dma_dev.dbg_dev_root, pt,
> + &pt_debugfs_info_fops);
> +
> + debugfs_create_file("stats", 0600, pt->dma_dev.dbg_dev_root, pt,

why 600 here?
--
~Vinod

2021-06-15 11:20:13

[permalink] [raw]

Subject: Re: [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA

On 6/9/2021 7:40 PM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 02-06-21, 12:22, Sanjay R Mehta wrote:
>
>> +/* DebugFS helpers */
>> +#define MAX_NAME_LEN 20
>> +#define RI_VERSION_NUM 0x0000003F
>> +
>> +#define RI_NUM_VQM 0x00078000
>> +#define RI_NVQM_SHIFT 15
>> +
>> +static DEFINE_MUTEX(pt_debugfs_lock);
>
> unused?
>
>> +
>> +static int pt_debugfs_info_show(struct seq_file *s, void *p)
>> +{
>> + struct pt_device *pt = s->private;
>> + unsigned int regval;
>> +
>> + if (!pt)
>> + return 0;
>
> better return an error code?
>
>> +
>> + seq_printf(s, "Device name: %s\n", pt->name);
>> + seq_printf(s, " # Queues: %d\n", 1);
>> + seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
>> +
>> + regval = ioread32(pt->io_regs + CMD_PT_VERSION);
>
> how do you ensure your device is not sleeping or you can access iomem
> safely?
>

This device will never go to sleep state as this DMA device is part of AMD server SOC.
Hence PM support is not implemented.

- Sanjay

2021-06-15 11:22:22

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 6/8/2021 11:09 PM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 02-06-21, 12:22, Sanjay R Mehta wrote:
>
>> +static int pt_core_execute_cmd(struct ptdma_desc *desc, struct pt_cmd_queue *cmd_q)
>> +{
>> + bool soc = FIELD_GET(DWORD0_SOC, desc->dw0);
>> + u8 *q_desc = (u8 *)&cmd_q->qbase[cmd_q->qidx];
>> + u8 *dp = (u8 *)desc;
>
> this case seems unnecessary?
>
>> +int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
>> + struct pt_passthru_engine *pt_engine)
>
> Pls align this to preceding open brace, checkpatch with --strict would
> warn you about this
>
>> +static irqreturn_t pt_core_irq_handler(int irq, void *data)
>> +{
>> + struct pt_device *pt = data;
>> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
>> + u32 status;
>> +
>> + pt_core_disable_queue_interrupts(pt);
>> +
>> + status = ioread32(cmd_q->reg_interrupt_status);
>> + if (status) {
>> + cmd_q->int_status = status;
>> + cmd_q->q_status = ioread32(cmd_q->reg_status);
>> + cmd_q->q_int_status = ioread32(cmd_q->reg_int_status);
>> +
>> + /* On error, only save the first error value */
>> + if ((status & INT_ERROR) && !cmd_q->cmd_error)
>> + cmd_q->cmd_error = CMD_Q_ERROR(cmd_q->q_status);
>> +
>> + /* Acknowledge the interrupt */
>> + iowrite32(status, cmd_q->reg_interrupt_status);
>> + }
>> +
>> + pt_core_enable_queue_interrupts(pt);
>> +
>> + return IRQ_HANDLED;
>
> should you always return IRQ_HANDLED, that sounds apt for the if loop
> but not for the non loop case
>
>> +int pt_core_init(struct pt_device *pt)
>> +{
>> + char dma_pool_name[MAX_DMAPOOL_NAME_LEN];
>> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
>> + u32 dma_addr_lo, dma_addr_hi;
>> + struct device *dev = pt->dev;
>> + struct dma_pool *dma_pool;
>> + int ret;
>> +
>> + /* Allocate a dma pool for the queue */
>> + snprintf(dma_pool_name, sizeof(dma_pool_name), "%s_q", pt->name);
>> +
>> + dma_pool = dma_pool_create(dma_pool_name, dev,
>> + PT_DMAPOOL_MAX_SIZE,
>> + PT_DMAPOOL_ALIGN, 0);
>> + if (!dma_pool) {
>> + dev_err(dev, "unable to allocate dma pool\n");
>
> This is superfluous, allocator would warn on failure
>
>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>> +{
>> + struct pt_device *pt;
>> +
>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>> +
>> + if (!pt)
>> + return NULL;
>> + pt->dev = dev;
>> + pt->ord = atomic_inc_return(&pt_ordinal);
>
> What is the use of this number?
>

There are eight similar instances of this DMA engine on AMD SOC.
It is to differentiate each of these instances.

- Sanjay

2021-06-16 04:18:37

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 15-06-21, 16:50, Sanjay R Mehta wrote:

> >> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >> +{
> >> + struct pt_device *pt;
> >> +
> >> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >> +
> >> + if (!pt)
> >> + return NULL;
> >> + pt->dev = dev;
> >> + pt->ord = atomic_inc_return(&pt_ordinal);
> >
> > What is the use of this number?
> >
>
> There are eight similar instances of this DMA engine on AMD SOC.
> It is to differentiate each of these instances.

Are they individual device objects?

--
~Vinod

2021-06-16 04:57:46

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 6/16/2021 9:45 AM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>
>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>>>> +{
>>>> + struct pt_device *pt;
>>>> +
>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>> +
>>>> + if (!pt)
>>>> + return NULL;
>>>> + pt->dev = dev;
>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>
>>> What is the use of this number?
>>>
>>
>> There are eight similar instances of this DMA engine on AMD SOC.
>> It is to differentiate each of these instances.
>
> Are they individual device objects?
>

Yes, they are individual device objects.

> --
> ~Vinod
>

2021-06-16 06:17:12

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> > [CAUTION: External Email]
> >
> > On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >
> >>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >>>> +{
> >>>> + struct pt_device *pt;
> >>>> +
> >>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>> +
> >>>> + if (!pt)
> >>>> + return NULL;
> >>>> + pt->dev = dev;
> >>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>
> >>> What is the use of this number?
> >>>
> >>
> >> There are eight similar instances of this DMA engine on AMD SOC.
> >> It is to differentiate each of these instances.
> >
> > Are they individual device objects?
> >
>
> Yes, they are individual device objects.

Then what is "ord" for? Why are you using an atomic variable for this?
What does this field do? Why doesn't the normal way of naming a device
come into play here instead?

thanks,

greg k-h

2021-06-16 06:58:27

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 6/16/2021 11:46 AM, Greg KH wrote:
> [CAUTION: External Email]
>
> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>>
>>
>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
>>> [CAUTION: External Email]
>>>
>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>>>
>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>>>>>> +{
>>>>>> + struct pt_device *pt;
>>>>>> +
>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>>>> +
>>>>>> + if (!pt)
>>>>>> + return NULL;
>>>>>> + pt->dev = dev;
>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>>>
>>>>> What is the use of this number?
>>>>>
>>>>
>>>> There are eight similar instances of this DMA engine on AMD SOC.
>>>> It is to differentiate each of these instances.
>>>
>>> Are they individual device objects?
>>>
>>
>> Yes, they are individual device objects.
>
> Then what is "ord" for? Why are you using an atomic variable for this?
> What does this field do? Why doesn't the normal way of naming a device
> come into play here instead?
>

Hi Greg,

The value of "ord" is incremented for each device instance and then it
is used to store different name for each device as shown in below snippet.

pt->ord = atomic_inc_return(&pt_ordinal);
snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);

- Sanjay

2021-06-16 07:18:11

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On Wed, Jun 16, 2021 at 12:27:32PM +0530, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 11:46 AM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> >>
> >>
> >> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> >>> [CAUTION: External Email]
> >>>
> >>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >>>
> >>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >>>>>> +{
> >>>>>> + struct pt_device *pt;
> >>>>>> +
> >>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>>>> +
> >>>>>> + if (!pt)
> >>>>>> + return NULL;
> >>>>>> + pt->dev = dev;
> >>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>>>
> >>>>> What is the use of this number?
> >>>>>
> >>>>
> >>>> There are eight similar instances of this DMA engine on AMD SOC.
> >>>> It is to differentiate each of these instances.
> >>>
> >>> Are they individual device objects?
> >>>
> >>
> >> Yes, they are individual device objects.
> >
> > Then what is "ord" for? Why are you using an atomic variable for this?
> > What does this field do? Why doesn't the normal way of naming a device
> > come into play here instead?
> >
>
> Hi Greg,
>
> The value of "ord" is incremented for each device instance and then it
> is used to store different name for each device as shown in below snippet.
>
> pt->ord = atomic_inc_return(&pt_ordinal);
> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);

Why not use an idr structure like this like all other drivers do? That
way when devices are removed the numbers are properly reused as well.

And why do you need to save the value?

thanks,

greg k-h

2021-06-16 07:54:19

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 16-06-21, 12:27, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 11:46 AM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> >>
> >>
> >> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> >>> [CAUTION: External Email]
> >>>
> >>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >>>
> >>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >>>>>> +{
> >>>>>> + struct pt_device *pt;
> >>>>>> +
> >>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>>>> +
> >>>>>> + if (!pt)
> >>>>>> + return NULL;
> >>>>>> + pt->dev = dev;
> >>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>>>
> >>>>> What is the use of this number?
> >>>>>
> >>>>
> >>>> There are eight similar instances of this DMA engine on AMD SOC.
> >>>> It is to differentiate each of these instances.
> >>>
> >>> Are they individual device objects?
> >>>
> >>
> >> Yes, they are individual device objects.
> >
> > Then what is "ord" for? Why are you using an atomic variable for this?
> > What does this field do? Why doesn't the normal way of naming a device
> > come into play here instead?
> >
>
> Hi Greg,
>
> The value of "ord" is incremented for each device instance and then it
> is used to store different name for each device as shown in below snippet.
>
> pt->ord = atomic_inc_return(&pt_ordinal);
> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);

Okay why not use device->name ?

Trying to unroll further, who creates pt_device? who creates the dev
object under this..?

Thanks
--
~Vinod

2021-06-16 08:03:15

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On Wed, Jun 16, 2021 at 01:22:54PM +0530, Vinod Koul wrote:
> On 16-06-21, 12:27, Sanjay R Mehta wrote:
> >
> >
> > On 6/16/2021 11:46 AM, Greg KH wrote:
> > > [CAUTION: External Email]
> > >
> > > On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> > >>
> > >>
> > >> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> > >>> [CAUTION: External Email]
> > >>>
> > >>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> > >>>
> > >>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)

In looking at this, why are you dealing with a "raw" struct device?
Shouldn't this be a parent pointer? Why not pass in the real type that
this can be made a child of?

> > >>>>>> +{
> > >>>>>> + struct pt_device *pt;
> > >>>>>> +
> > >>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> > >>>>>> +
> > >>>>>> + if (!pt)
> > >>>>>> + return NULL;
> > >>>>>> + pt->dev = dev;
> > >>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> > >>>>>
> > >>>>> What is the use of this number?
> > >>>>>
> > >>>>
> > >>>> There are eight similar instances of this DMA engine on AMD SOC.
> > >>>> It is to differentiate each of these instances.
> > >>>
> > >>> Are they individual device objects?
> > >>>
> > >>
> > >> Yes, they are individual device objects.
> > >
> > > Then what is "ord" for? Why are you using an atomic variable for this?
> > > What does this field do? Why doesn't the normal way of naming a device
> > > come into play here instead?
> > >
> >
> > Hi Greg,
> >
> > The value of "ord" is incremented for each device instance and then it
> > is used to store different name for each device as shown in below snippet.
> >
> > pt->ord = atomic_inc_return(&pt_ordinal);
> > snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
>
> Okay why not use device->name ?

Ah, I missed this. Yes, do not have 2 names for the same structure,
that is wasteful and confusing.

thanks,

greg k-h

2021-06-16 09:49:44

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 6/16/2021 1:29 PM, Greg KH wrote:
> [CAUTION: External Email]
>
> On Wed, Jun 16, 2021 at 01:22:54PM +0530, Vinod Koul wrote:
>> On 16-06-21, 12:27, Sanjay R Mehta wrote:
>>>
>>>
>>> On 6/16/2021 11:46 AM, Greg KH wrote:
>>>> [CAUTION: External Email]
>>>>
>>>> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>>>>>
>>>>>
>>>>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
>>>>>> [CAUTION: External Email]
>>>>>>
>>>>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>>>>>>
>>>>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>
> In looking at this, why are you dealing with a "raw" struct device?
> Shouldn't this be a parent pointer? Why not pass in the real type that
> this can be made a child of?
>
>
>>>>>>>>> +{
>>>>>>>>> + struct pt_device *pt;
>>>>>>>>> +
>>>>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>>>>>>> +
>>>>>>>>> + if (!pt)
>>>>>>>>> + return NULL;
>>>>>>>>> + pt->dev = dev;
>>>>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>>>>>>
>>>>>>>> What is the use of this number?
>>>>>>>>
>>>>>>>
>>>>>>> There are eight similar instances of this DMA engine on AMD SOC.
>>>>>>> It is to differentiate each of these instances.
>>>>>>
>>>>>> Are they individual device objects?
>>>>>>
>>>>>
>>>>> Yes, they are individual device objects.
>>>>
>>>> Then what is "ord" for? Why are you using an atomic variable for this?
>>>> What does this field do? Why doesn't the normal way of naming a device
>>>> come into play here instead?
>>>>
>>>
>>> Hi Greg,
>>>
>>> The value of "ord" is incremented for each device instance and then it
>>> is used to store different name for each device as shown in below snippet.
>>>
>>> pt->ord = atomic_inc_return(&pt_ordinal);
>>> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
>>
>> Okay why not use device->name ?
>
> Ah, I missed this. Yes, do not have 2 names for the same structure,
> that is wasteful and confusing.
>

Thanks, Greg & Vinod. I just verified with "dev_name(dev)" and this is
serving the purpose :).

I will send this change in the next version.

- Sanjay

2021-06-16 09:58:21

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 16-06-21, 15:16, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 1:29 PM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 01:22:54PM +0530, Vinod Koul wrote:
> >> On 16-06-21, 12:27, Sanjay R Mehta wrote:
> >>>
> >>>
> >>> On 6/16/2021 11:46 AM, Greg KH wrote:
> >>>> [CAUTION: External Email]
> >>>>
> >>>> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> >>>>>
> >>>>>
> >>>>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> >>>>>> [CAUTION: External Email]
> >>>>>>
> >>>>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >>>>>>
> >>>>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >
> > In looking at this, why are you dealing with a "raw" struct device?
> > Shouldn't this be a parent pointer? Why not pass in the real type that
> > this can be made a child of?
> >
> >
> >>>>>>>>> +{
> >>>>>>>>> + struct pt_device *pt;
> >>>>>>>>> +
> >>>>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>>>>>>> +
> >>>>>>>>> + if (!pt)
> >>>>>>>>> + return NULL;
> >>>>>>>>> + pt->dev = dev;
> >>>>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>>>>>>
> >>>>>>>> What is the use of this number?
> >>>>>>>>
> >>>>>>>
> >>>>>>> There are eight similar instances of this DMA engine on AMD SOC.
> >>>>>>> It is to differentiate each of these instances.
> >>>>>>
> >>>>>> Are they individual device objects?
> >>>>>>
> >>>>>
> >>>>> Yes, they are individual device objects.
> >>>>
> >>>> Then what is "ord" for? Why are you using an atomic variable for this?
> >>>> What does this field do? Why doesn't the normal way of naming a device
> >>>> come into play here instead?
> >>>>
> >>>
> >>> Hi Greg,
> >>>
> >>> The value of "ord" is incremented for each device instance and then it
> >>> is used to store different name for each device as shown in below snippet.
> >>>
> >>> pt->ord = atomic_inc_return(&pt_ordinal);
> >>> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
> >>
> >> Okay why not use device->name ?
> >
> > Ah, I missed this. Yes, do not have 2 names for the same structure,
> > that is wasteful and confusing.
> >
>
> Thanks, Greg & Vinod. I just verified with "dev_name(dev)" and this is
> serving the purpose :).
>
> I will send this change in the next version.

Great, but there are few more questions I had, like who creates the
device etc, can you please respond to those questions as well, so that
we understand properly how this device works

Thanks
--
~Vinod

2021-06-16 13:23:13

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 6/16/2021 1:22 PM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 16-06-21, 12:27, Sanjay R Mehta wrote:
>>
>>
>> On 6/16/2021 11:46 AM, Greg KH wrote:
>>> [CAUTION: External Email]
>>>
>>> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>>>>
>>>>
>>>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
>>>>> [CAUTION: External Email]
>>>>>
>>>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>>>>>
>>>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>>>>>>>> +{
>>>>>>>> + struct pt_device *pt;
>>>>>>>> +
>>>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>>>>>> +
>>>>>>>> + if (!pt)
>>>>>>>> + return NULL;
>>>>>>>> + pt->dev = dev;
>>>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>>>>>
>>>>>>> What is the use of this number?
>>>>>>>
>>>>>>
>>>>>> There are eight similar instances of this DMA engine on AMD SOC.
>>>>>> It is to differentiate each of these instances.
>>>>>
>>>>> Are they individual device objects?
>>>>>
>>>>
>>>> Yes, they are individual device objects.
>>>
>>> Then what is "ord" for? Why are you using an atomic variable for this?
>>> What does this field do? Why doesn't the normal way of naming a device
>>> come into play here instead?
>>>
>>
>> Hi Greg,
>>
>> The value of "ord" is incremented for each device instance and then it
>> is used to store different name for each device as shown in below snippet.
>>
>> pt->ord = atomic_inc_return(&pt_ordinal);
>> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
>
> Okay why not use device->name ?
>
> Trying to unroll further, who creates pt_device? who creates the dev
> object under this..?
>

Hi Vinod,

The pt_device is allocated and initialized in the PCI probe function and
then we just get the "dev" from the "pci_dev" object and save it in
"pt->dev" as shown in below snippet.

static int pt_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
{
struct pt_device *pt;
struct pt_msix *pt_msix;
struct device *dev = &pdev->dev;

Thanks,
- Sanjay

2021-06-16 13:27:46

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On Wed, Jun 16, 2021 at 05:30:49PM +0530, Sanjay R Mehta wrote:
> The pt_device is allocated and initialized in the PCI probe function and
> then we just get the "dev" from the "pci_dev" object and save it in
> "pt->dev" as shown in below snippet.
>
>
> static int pt_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> {
> struct pt_device *pt;
> struct pt_msix *pt_msix;
> struct device *dev = &pdev->dev;

So "dev" is a parent here, or something else?

If it is the parent, please call it such otherwise it is confusing.

If you are creating child devices, what bus do they belong to?

Can you fix up this series and resend it so that we can review it again?

thanks,

greg k-h

2021-06-16 13:40:06

[permalink] [raw]

Subject: Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

On 6/16/2021 5:53 PM, Greg KH wrote:
> [CAUTION: External Email]
>
> On Wed, Jun 16, 2021 at 05:30:49PM +0530, Sanjay R Mehta wrote:
>> The pt_device is allocated and initialized in the PCI probe function and
>> then we just get the "dev" from the "pci_dev" object and save it in
>> "pt->dev" as shown in below snippet.
>>
>>
>> static int pt_pci_probe(struct pci_dev *pdev, const struct
>> pci_device_id *id)
>> {
>> struct pt_device *pt;
>> struct pt_msix *pt_msix;
>> struct device *dev = &pdev->dev;
>
> So "dev" is a parent here, or something else?
>
> If it is the parent, please call it such otherwise it is confusing.
>
> If you are creating child devices, what bus do they belong to?
>
> Can you fix up this series and resend it so that we can review it again?
>

Hi Greg,

Yes, "dev" is the parent here and there are no child devices created.

My apologies for not calling it rightly.

Sure, I will fix up this series addressing all the comments and will
send the next version.

Thanks,
-Sanjay

2021-06-16 13:40:23