Dma mrpc by wesleywesley · Pull Request #12 · Microsemi/switchtec-kernel

wesleywesley · 2018-05-30T09:48:26Z

No description provided.

lsgunth

Please also add some better description to the commit message. Especially if you intend on pushing this upstream at some point.

lsgunth · 2018-05-30T16:46:36Z

 	struct list_head mrpc_queue;
 	int mrpc_busy;
 	struct work_struct mrpc_work;
+	struct work_struct dma_mrpc_work;


Adding registers in a separate patch is a good idea, but adding fields to a structure that you don't use yet is not good practice. Add them when you use them.

lsgunth · 2018-05-30T16:47:13Z


 	struct switchtec_ntb *sndev;
+
+	bool dma_mrpc;


I think this extra bool is unnecessary. Just check if mrpc_output_addr is NULL or not.

lsgunth · 2018-05-30T16:57:53Z

+
+	bool dma_mrpc;
+	struct dma_mrpc_output *mrpc_output_addr;
+	dma_addr_t mrpc_output_dma;


I don't like the names chosen. mrpc_output_addr is a pointer to a struct, not an address. If we prefix every pointer with _addr then there will be a lot of noise. It also doesn't indicate that this struct is only used for MRPC in DMA mode. Where as mrpc_output_dma is an address...

I'd suggest something like:

struct dma_mrpc_output *dma_mrpc;
dma_addr_t dma_mrpc_dma_addr;

lsgunth · 2018-05-30T16:59:42Z

+}
+#endif
+#endif
+


Lets not propagate this mess any further than it already is. I've been trying to remove it.

For now, just use readq()/writeq()

lsgunth · 2018-05-30T17:05:43Z

 }

+static void dma_mrpc_complete_cmd(struct switchtec_dev *stdev)
+{


This function is almost identical to mrpc_complete_cmd(). Never cut and paste code. Better to just check if we are doing a DMA transaction and copy it from where appropriate.

Then, I think this should remove the need for the separate work queues, etc.

Will combine the two work items and completion functions.

lsgunth · 2018-05-30T17:10:52Z

+		return rc;
+
+	if (stdev->dma_mrpc){
+		event_irq = ioread32(&stdev->mmio_mrpc->dma_vector);


We should rename event_irq as it now is used for both the event IRQ and the DMA IRQ.

lsgunth · 2018-05-30T17:12:07Z

 }

+
+static irqreturn_t switchtec_mrpc_isr(int irq, void *dev)


This function name is confusing. It should probably include "dma" somewhere in there. It's assigned to an interrupt the hardware seems to refer to as "dma_vector"... It's not a general MRPC interrupt.

rename to switchtec_dma_mrpc_isr

lsgunth · 2018-05-30T17:22:16Z

@@ -1022,7 +1109,11 @@ static void enable_link_state_events(struct switchtec_dev *stdev)
 static void stdev_release(struct device *dev)
 {
 	struct switchtec_dev *stdev = to_stdev(dev);


Style nit: a space should always come after the variables are declared.

lsgunth · 2018-05-30T17:24:25Z

 err_devadd:
 	stdev_kill(stdev);
 err_put:
+	stdev_release(&stdev->dev);


This is wrong and will cause a use after free bug. Notice that stdev_release() will free stdev and the next couple lines then use stdev. In fact stdev_release() is already called only after the last reference to the device is freed. So put_device() may or may not call it depending on if something else still holds a reference to it.

lsgunth · 2018-05-30T17:24:33Z

 	dev_info(&stdev->dev, "unregistered.\n");
-
 	stdev_kill(stdev);
+	stdev_release(&stdev->dev);


Also wrong.

lsgunth · 2018-05-30T17:52:33Z

+	if (stdev->dma_mrpc){
+		iowrite64(stdev->mrpc_output_dma, &stdev->mmio_mrpc->dma_addr);
+		iowrite32(SWITCHTEC_DMA_MRPC_EN_IRQ, &stdev->mmio_mrpc->dma_en);
+	}


Also: very important -- We need to remember to unset dma_addr before we call dma_free_coherent(). We don't want the hardware writing to that memory after it's freed.

It's also not clear to me what enables using DMA transfers. You only seem to set the flag to enable interrupt requests. Does the hardware enable DMA requests once dma_addr is set or is the EN_IRQ flag mis-named?

Also, you might want to consider some of the security implications here. On Windows, any user can now write this DMA address and can use it to scribble on any kernel-space memory. That's really bad. Makes me regret letting your colleagues convince me to allow userspace access to GAS at all... At least on Linux it requires root access and the driver doesn't actually add any privilege userspace didn't already have. Someone should probably look into ensuring GAS access on Windows at least requires elevated privileges, but I'm not sure how that can be done.

Nice capture.
For the dma mrpc new feature: four registers added: enable, address, vector and version.
where version number greater than zero, signify dma mrpc feature is supported; dma mrpc enabled by enable register instead of filling the target address to address register. So, yes, EN_IRQ flag msi-named.
More about the IRQ:
Command register format also change for this new feature.
Where BIT [15:0] keep unchanged, should feed with mrpc command index.
BIT 16, notify or not, where 0 notify, 1 not notify.
Whether the event will trig IRQ or not, is still controlled by BIT 3 of MRPC completion header reg.

lsgunth · 2018-05-31T15:36:40Z

-	memcpy_fromio(stuser->data, &stdev->mmio_mrpc->output_data,
-		      stuser->read_len);
+	if (stdev->dma_mrpc)
+		memcpy_fromio(stuser->data, &stdev->dma_mrpc->data,


This should be a regular memcpy

it should be memcpy instead of memcpy_fromio.

lsgunth · 2018-05-31T15:38:15Z

 		return event_irq;

-	return devm_request_irq(&stdev->pdev->dev, event_irq,
+	rc =  devm_request_irq(&stdev->pdev->dev, event_irq,


Style nit: There's one too many spaces between = and devm

lsgunth · 2018-05-31T15:38:57Z

+		return 0;
+
+	stdev->dma_mrpc = dma_zalloc_coherent(&stdev->pdev->dev, sizeof(*stdev->dma_mrpc),
+								&stdev->dma_mrpc_dma_addr, GFP_KERNEL);


Style nit: indentation should align with the open parenthesis

lsgunth · 2018-05-31T15:39:52Z

+	if (stdev->dma_mrpc){
+		dma_free_coherent(&stdev->pdev->dev, sizeof(*stdev->dma_mrpc),
+				stdev->dma_mrpc, stdev->dma_mrpc_dma_addr);
+		iowrite32(0, &stdev->mmio_mrpc->dma_en);


I think it's best we also clear the address as well as the enable.

confirm that device side treat dma_mrpc_dma_addr 0 as a invalid address value, none dma operation will executed in 0 value case. So we can deinit the register by 0.
But from PCIe aspect, it is not explicit said in spec, the address 0 is a invalid field in memory write tlp. This is why I hesitate to add clear the address by 0, but just disable dma function

If something goes wrong I'd rather see and have to debug a write to an invalid address of zero than to a valid physical address that the OS isn't expected. The former will at worst produce an error while the latter will be scribbling on random memory in the system which would be a nightmare if it has to be debugged.

1. macro definition for dma mrpc enable bit 2. registers include: enable, dma target address, vector, and version 3. data structure of dma mrpc output

wesleywesley · 2018-06-02T03:33:06Z

Hi, Logan, Kelvin, and Joey,

I go through all the comments in this conversations again, it seems that all the suggestion have been adopted.
And also the action items for code review, they are already done.

If no more comments/suggestions, I will merge the pull request into devel branch.

Thank you all for the help and the good/excellent suggestions.

regards,
Wesley

lsgunth

Besides the two kernel style nits I noticed this looks good.

lsgunth · 2018-06-04T16:03:36Z

 {
 	struct switchtec_dev *stdev = to_stdev(dev);
-
+	if (stdev->dma_mrpc){


Style nit: There should be a blank line after variables are declared.

fixed, thx.

lsgunth · 2018-06-04T16:04:18Z

 		  &stdev->mmio_part_cfg->mrpc_comp_hdr);
 	enable_link_state_events(stdev);

+	if (stdev->dma_mrpc){


Style nit: Single line if statements should not have curly brackets.

fixed, thx.

1. add dma output target virtual and bus addrss member in switchtec_dev struct 2. in mrpc completion function, collect the result from different source 3. release dma output buffer and disable mrpc dma function in stdev_release 4. add isr for dma mrpc 5. according to the dma version register value, allocate dma buffer, enable dma function and register interrupt service routine

dma mrpc or legacy mrpc is controllable by this parameter

wesleywesley requested review from a user, kelvin-cao and lsgunth May 30, 2018 09:48

lsgunth requested changes May 30, 2018

View reviewed changes

lsgunth reviewed May 30, 2018

View reviewed changes

wesleywesley force-pushed the dma_mrpc branch from 75db4cb to 79f2575 Compare May 31, 2018 10:29

lsgunth requested changes May 31, 2018

View reviewed changes

wesleywesley force-pushed the dma_mrpc branch 2 times, most recently from 9c76d92 to 4e6a3b4 Compare June 1, 2018 07:02

macro, registers and data structure for dma mrpc

6827153

1. macro definition for dma mrpc enable bit 2. registers include: enable, dma target address, vector, and version 3. data structure of dma mrpc output

lsgunth approved these changes Jun 4, 2018

View reviewed changes

wesleywesley force-pushed the dma_mrpc branch from 17d83a4 to 1c90fd7 Compare June 5, 2018 02:23

wesleywesley added 2 commits June 5, 2018 18:19

add module parameter use_dma_mrpc

1c90fd7

dma mrpc or legacy mrpc is controllable by this parameter

wesleywesley merged commit 70823c4 into devel Jun 6, 2018

wesleywesley deleted the dma_mrpc branch August 9, 2018 07:32

msmith626 mentioned this pull request Feb 12, 2019

BUG: unable to handle kernel paging request at ffffc9000a1f0000 #59

Closed

Conversation

wesleywesley commented May 30, 2018

Uh oh!

lsgunth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesleywesley commented Jun 2, 2018

Uh oh!

lsgunth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lsgunth Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lsgunth Jun 4, 2018 •

edited

Loading