diff for duplicates of <000201d2e6cb$9f4f4e50$ddedeaf0$@dell.com> diff --git a/a/1.txt b/N1/1.txt index c8eaed2..a25a4be 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,88 +1,62 @@ From: Logan Gunthorpe > On 16/06/17 09:34 AM, Allen Hubbe wrote: -> > In code review, I really only have found minor nits. Overall, the = -driver looks good. ->=20 +> > In code review, I really only have found minor nits. Overall, the driver looks good. +> > Great, thanks for such a quick review! ->=20 -> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * = -50ms). This looks like a thread -> context, so it could involve the scheduler for the delay instead of = -spinning for up to 50s before +> +> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * 50ms). This looks like a thread +> context, so it could involve the scheduler for the delay instead of spinning for up to 50s before > bailing. ->=20 -> Good point. If I were to change this to msleep_interruptible would = -that +> +> Good point. If I were to change this to msleep_interruptible would that > be acceptable? I would be satisfied. ->=20 +> > > There are a few instances like this: > >> + dev_dbg(&stdev->dev, "%s\n", __func__); ->=20 -> > Where the printing of __func__ could be controlled by dyndbg=3D+pf. = -The debug message could be more +> +> > Where the printing of __func__ could be controlled by dyndbg=+pf. The debug message could be more > useful. ->=20 +> > Ok, I'll change that. ->=20 -> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the = -mask bits is protected by a -> spinlock. Elsewhere, you noted that the db bits are shared between = -all ports, so the db bitset is -> chopped up to be shared between the ports. Is the db mask also = -shared, and how is the spinlock -> sufficient for synchronizing access to the mask bits between multiple = -ports? ->=20 -> Well, there are 64 doorbells that are shared between ports but each = -port +> +> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the mask bits is protected by a +> spinlock. Elsewhere, you noted that the db bits are shared between all ports, so the db bitset is +> chopped up to be shared between the ports. Is the db mask also shared, and how is the spinlock +> sufficient for synchronizing access to the mask bits between multiple ports? +> +> Well, there are 64 doorbells that are shared between ports but each port > has it's own in and out registers for the doorbells. So triggering -> doorbell one on one port's ODB actually triggers it on every ports = -IDB. -> So these are shared only in the sense that each port needs to know = -which -> dbs it cares about. Seeing each port has their own registers they = -don't +> doorbell one on one port's ODB actually triggers it on every ports IDB. +> So these are shared only in the sense that each port needs to know which +> dbs it cares about. Seeing each port has their own registers they don't > have to worry about synchronization. ->=20 +> > The mask is only protected by a spin lock seeing multiple callers of > db_set_mask and db_clr_mask on the same port may step on each others -> toes. So if two processes try to mask different bits they both must = -get +> toes. So if two processes try to mask different bits they both must get > masked in the end and therefore some kind of synchronization must be > involved. -Thanks for clearing that up. Now I understand, each port has its own = -independent set of mask bits. So, while the doorbell numbers are = -assigned globally, the registers themselves are per port. For the mask = -bits, the mask behavior only affects the local port. +Thanks for clearing that up. Now I understand, each port has its own independent set of mask bits. So, while the doorbell numbers are assigned globally, the registers themselves are per port. For the mask bits, the mask behavior only affects the local port. ->=20 -> > The IDT switch also does not have hardware scratchpads. Could the = -code you wrote for emulated -> scratchpads be made into shared library code for ntb drivers? Also, = -some ntb clients may not need -> scratchpad support. If it is not natively supported by a driver, can = -the emulated scratchpad support +> +> > The IDT switch also does not have hardware scratchpads. Could the code you wrote for emulated +> scratchpads be made into shared library code for ntb drivers? Also, some ntb clients may not need +> scratchpad support. If it is not natively supported by a driver, can the emulated scratchpad support > be an optional feature? ->=20 -> Hmm, interesting idea. A few pieces could possibly be made common but = -it +> +> Hmm, interesting idea. A few pieces could possibly be made common but it > depends mostly on hardware having the resources to make use of it. > Switchtec has extra LUT memory windows that made this possible. Unless -> you object I'm inclined to leave it as is and I'd be happy to work = -with +> you object I'm inclined to leave it as is and I'd be happy to work with > the IDT folks to create a common solution in the future. -Alright. I'll leave it to you to find and reconcile common = -functionalities of the drivers. What about making spad emulation = -optional? +Alright. I'll leave it to you to find and reconcile common functionalities of the drivers. What about making spad emulation optional? ->=20 +> > Logan -There was a comment on irc.oftc.net #ntb wishing for patch v2 to be = -fewer patches. Something like, 1/2: prep the existing switch driver, = -2/2: introduce the ntb driver. +There was a comment on irc.oftc.net #ntb wishing for patch v2 to be fewer patches. Something like, 1/2: prep the existing switch driver, 2/2: introduce the ntb driver. diff --git a/a/content_digest b/N1/content_digest index 19ff0e5..1b08cb0 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -7,9 +7,9 @@ "Subject\0RE: [RFC PATCH 00/13] Switchtec NTB Support\0" "Date\0Fri, 16 Jun 2017 14:08:52 -0400\0" "To\0'Logan Gunthorpe' <logang@deltatee.com>" - <linux-ntb@googlegroups.com> - <linux-pci@vger.kernel.org> - " <linux-kernel@vger.kernel.org>\0" + linux-ntb@googlegroups.com + linux-pci@vger.kernel.org + " linux-kernel@vger.kernel.org\0" "Cc\0'Jon Mason' <jdmason@kudzu.us>" 'Dave Jiang' <dave.jiang@intel.com> 'Bjorn Helgaas' <bhelgaas@google.com> @@ -17,96 +17,70 @@ 'Kurt Schwemmer' <kurt.schwemmer@microsemi.com> 'Stephen Bates' <sbates@raithlin.com> 'Serge Semin' <fancer.lancer@gmail.com> - " <Sergey.Semin@t-platforms.ru>\0" + " Sergey.Semin@t-platforms.ru\0" "\00:1\0" "b\0" "From: Logan Gunthorpe\n" "> On 16/06/17 09:34 AM, Allen Hubbe wrote:\n" - "> > In code review, I really only have found minor nits. Overall, the =\n" - "driver looks good.\n" - ">=20\n" + "> > In code review, I really only have found minor nits. Overall, the driver looks good.\n" + "> \n" "> Great, thanks for such a quick review!\n" - ">=20\n" - "> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * =\n" - "50ms). This looks like a thread\n" - "> context, so it could involve the scheduler for the delay instead of =\n" - "spinning for up to 50s before\n" + "> \n" + "> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * 50ms). This looks like a thread\n" + "> context, so it could involve the scheduler for the delay instead of spinning for up to 50s before\n" "> bailing.\n" - ">=20\n" - "> Good point. If I were to change this to msleep_interruptible would =\n" - "that\n" + "> \n" + "> Good point. If I were to change this to msleep_interruptible would that\n" "> be acceptable?\n" "\n" "I would be satisfied.\n" "\n" - ">=20\n" + "> \n" "> > There are a few instances like this:\n" "> >> +\tdev_dbg(&stdev->dev, \"%s\\n\", __func__);\n" - ">=20\n" - "> > Where the printing of __func__ could be controlled by dyndbg=3D+pf. =\n" - "The debug message could be more\n" + "> \n" + "> > Where the printing of __func__ could be controlled by dyndbg=+pf. The debug message could be more\n" "> useful.\n" - ">=20\n" + "> \n" "> Ok, I'll change that.\n" - ">=20\n" - "> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the =\n" - "mask bits is protected by a\n" - "> spinlock. Elsewhere, you noted that the db bits are shared between =\n" - "all ports, so the db bitset is\n" - "> chopped up to be shared between the ports. Is the db mask also =\n" - "shared, and how is the spinlock\n" - "> sufficient for synchronizing access to the mask bits between multiple =\n" - "ports?\n" - ">=20\n" - "> Well, there are 64 doorbells that are shared between ports but each =\n" - "port\n" + "> \n" + "> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the mask bits is protected by a\n" + "> spinlock. Elsewhere, you noted that the db bits are shared between all ports, so the db bitset is\n" + "> chopped up to be shared between the ports. Is the db mask also shared, and how is the spinlock\n" + "> sufficient for synchronizing access to the mask bits between multiple ports?\n" + "> \n" + "> Well, there are 64 doorbells that are shared between ports but each port\n" "> has it's own in and out registers for the doorbells. So triggering\n" - "> doorbell one on one port's ODB actually triggers it on every ports =\n" - "IDB.\n" - "> So these are shared only in the sense that each port needs to know =\n" - "which\n" - "> dbs it cares about. Seeing each port has their own registers they =\n" - "don't\n" + "> doorbell one on one port's ODB actually triggers it on every ports IDB.\n" + "> So these are shared only in the sense that each port needs to know which\n" + "> dbs it cares about. Seeing each port has their own registers they don't\n" "> have to worry about synchronization.\n" - ">=20\n" + "> \n" "> The mask is only protected by a spin lock seeing multiple callers of\n" "> db_set_mask and db_clr_mask on the same port may step on each others\n" - "> toes. So if two processes try to mask different bits they both must =\n" - "get\n" + "> toes. So if two processes try to mask different bits they both must get\n" "> masked in the end and therefore some kind of synchronization must be\n" "> involved.\n" "\n" - "Thanks for clearing that up. Now I understand, each port has its own =\n" - "independent set of mask bits. So, while the doorbell numbers are =\n" - "assigned globally, the registers themselves are per port. For the mask =\n" - "bits, the mask behavior only affects the local port.\n" + "Thanks for clearing that up. Now I understand, each port has its own independent set of mask bits. So, while the doorbell numbers are assigned globally, the registers themselves are per port. For the mask bits, the mask behavior only affects the local port.\n" "\n" - ">=20\n" - "> > The IDT switch also does not have hardware scratchpads. Could the =\n" - "code you wrote for emulated\n" - "> scratchpads be made into shared library code for ntb drivers? Also, =\n" - "some ntb clients may not need\n" - "> scratchpad support. If it is not natively supported by a driver, can =\n" - "the emulated scratchpad support\n" + "> \n" + "> > The IDT switch also does not have hardware scratchpads. Could the code you wrote for emulated\n" + "> scratchpads be made into shared library code for ntb drivers? Also, some ntb clients may not need\n" + "> scratchpad support. If it is not natively supported by a driver, can the emulated scratchpad support\n" "> be an optional feature?\n" - ">=20\n" - "> Hmm, interesting idea. A few pieces could possibly be made common but =\n" - "it\n" + "> \n" + "> Hmm, interesting idea. A few pieces could possibly be made common but it\n" "> depends mostly on hardware having the resources to make use of it.\n" "> Switchtec has extra LUT memory windows that made this possible. Unless\n" - "> you object I'm inclined to leave it as is and I'd be happy to work =\n" - "with\n" + "> you object I'm inclined to leave it as is and I'd be happy to work with\n" "> the IDT folks to create a common solution in the future.\n" "\n" - "Alright. I'll leave it to you to find and reconcile common =\n" - "functionalities of the drivers. What about making spad emulation =\n" - "optional?\n" + "Alright. I'll leave it to you to find and reconcile common functionalities of the drivers. What about making spad emulation optional?\n" "\n" - ">=20\n" + "> \n" "> Logan\n" "\n" - "There was a comment on irc.oftc.net #ntb wishing for patch v2 to be =\n" - "fewer patches. Something like, 1/2: prep the existing switch driver, =\n" - 2/2: introduce the ntb driver. + There was a comment on irc.oftc.net #ntb wishing for patch v2 to be fewer patches. Something like, 1/2: prep the existing switch driver, 2/2: introduce the ntb driver. -d85b25c4777300a36a89e49ddc61047b62ab658858bbcff93fc4a72c29891f12 +2e5029dcda420537439bb787342fbb80fdfb42cd26be35e301da5fc7b3d4d81f
diff --git a/a/1.txt b/N2/1.txt index c8eaed2..a25a4be 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,88 +1,62 @@ From: Logan Gunthorpe > On 16/06/17 09:34 AM, Allen Hubbe wrote: -> > In code review, I really only have found minor nits. Overall, the = -driver looks good. ->=20 +> > In code review, I really only have found minor nits. Overall, the driver looks good. +> > Great, thanks for such a quick review! ->=20 -> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * = -50ms). This looks like a thread -> context, so it could involve the scheduler for the delay instead of = -spinning for up to 50s before +> +> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * 50ms). This looks like a thread +> context, so it could involve the scheduler for the delay instead of spinning for up to 50s before > bailing. ->=20 -> Good point. If I were to change this to msleep_interruptible would = -that +> +> Good point. If I were to change this to msleep_interruptible would that > be acceptable? I would be satisfied. ->=20 +> > > There are a few instances like this: > >> + dev_dbg(&stdev->dev, "%s\n", __func__); ->=20 -> > Where the printing of __func__ could be controlled by dyndbg=3D+pf. = -The debug message could be more +> +> > Where the printing of __func__ could be controlled by dyndbg=+pf. The debug message could be more > useful. ->=20 +> > Ok, I'll change that. ->=20 -> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the = -mask bits is protected by a -> spinlock. Elsewhere, you noted that the db bits are shared between = -all ports, so the db bitset is -> chopped up to be shared between the ports. Is the db mask also = -shared, and how is the spinlock -> sufficient for synchronizing access to the mask bits between multiple = -ports? ->=20 -> Well, there are 64 doorbells that are shared between ports but each = -port +> +> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the mask bits is protected by a +> spinlock. Elsewhere, you noted that the db bits are shared between all ports, so the db bitset is +> chopped up to be shared between the ports. Is the db mask also shared, and how is the spinlock +> sufficient for synchronizing access to the mask bits between multiple ports? +> +> Well, there are 64 doorbells that are shared between ports but each port > has it's own in and out registers for the doorbells. So triggering -> doorbell one on one port's ODB actually triggers it on every ports = -IDB. -> So these are shared only in the sense that each port needs to know = -which -> dbs it cares about. Seeing each port has their own registers they = -don't +> doorbell one on one port's ODB actually triggers it on every ports IDB. +> So these are shared only in the sense that each port needs to know which +> dbs it cares about. Seeing each port has their own registers they don't > have to worry about synchronization. ->=20 +> > The mask is only protected by a spin lock seeing multiple callers of > db_set_mask and db_clr_mask on the same port may step on each others -> toes. So if two processes try to mask different bits they both must = -get +> toes. So if two processes try to mask different bits they both must get > masked in the end and therefore some kind of synchronization must be > involved. -Thanks for clearing that up. Now I understand, each port has its own = -independent set of mask bits. So, while the doorbell numbers are = -assigned globally, the registers themselves are per port. For the mask = -bits, the mask behavior only affects the local port. +Thanks for clearing that up. Now I understand, each port has its own independent set of mask bits. So, while the doorbell numbers are assigned globally, the registers themselves are per port. For the mask bits, the mask behavior only affects the local port. ->=20 -> > The IDT switch also does not have hardware scratchpads. Could the = -code you wrote for emulated -> scratchpads be made into shared library code for ntb drivers? Also, = -some ntb clients may not need -> scratchpad support. If it is not natively supported by a driver, can = -the emulated scratchpad support +> +> > The IDT switch also does not have hardware scratchpads. Could the code you wrote for emulated +> scratchpads be made into shared library code for ntb drivers? Also, some ntb clients may not need +> scratchpad support. If it is not natively supported by a driver, can the emulated scratchpad support > be an optional feature? ->=20 -> Hmm, interesting idea. A few pieces could possibly be made common but = -it +> +> Hmm, interesting idea. A few pieces could possibly be made common but it > depends mostly on hardware having the resources to make use of it. > Switchtec has extra LUT memory windows that made this possible. Unless -> you object I'm inclined to leave it as is and I'd be happy to work = -with +> you object I'm inclined to leave it as is and I'd be happy to work with > the IDT folks to create a common solution in the future. -Alright. I'll leave it to you to find and reconcile common = -functionalities of the drivers. What about making spad emulation = -optional? +Alright. I'll leave it to you to find and reconcile common functionalities of the drivers. What about making spad emulation optional? ->=20 +> > Logan -There was a comment on irc.oftc.net #ntb wishing for patch v2 to be = -fewer patches. Something like, 1/2: prep the existing switch driver, = -2/2: introduce the ntb driver. +There was a comment on irc.oftc.net #ntb wishing for patch v2 to be fewer patches. Something like, 1/2: prep the existing switch driver, 2/2: introduce the ntb driver. diff --git a/a/content_digest b/N2/content_digest index 19ff0e5..151f8c8 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -22,91 +22,65 @@ "b\0" "From: Logan Gunthorpe\n" "> On 16/06/17 09:34 AM, Allen Hubbe wrote:\n" - "> > In code review, I really only have found minor nits. Overall, the =\n" - "driver looks good.\n" - ">=20\n" + "> > In code review, I really only have found minor nits. Overall, the driver looks good.\n" + "> \n" "> Great, thanks for such a quick review!\n" - ">=20\n" - "> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * =\n" - "50ms). This looks like a thread\n" - "> context, so it could involve the scheduler for the delay instead of =\n" - "spinning for up to 50s before\n" + "> \n" + "> > In switchtec_ntb_part_op, there is a delay of up to 50s (1000 * 50ms). This looks like a thread\n" + "> context, so it could involve the scheduler for the delay instead of spinning for up to 50s before\n" "> bailing.\n" - ">=20\n" - "> Good point. If I were to change this to msleep_interruptible would =\n" - "that\n" + "> \n" + "> Good point. If I were to change this to msleep_interruptible would that\n" "> be acceptable?\n" "\n" "I would be satisfied.\n" "\n" - ">=20\n" + "> \n" "> > There are a few instances like this:\n" "> >> +\tdev_dbg(&stdev->dev, \"%s\\n\", __func__);\n" - ">=20\n" - "> > Where the printing of __func__ could be controlled by dyndbg=3D+pf. =\n" - "The debug message could be more\n" + "> \n" + "> > Where the printing of __func__ could be controlled by dyndbg=+pf. The debug message could be more\n" "> useful.\n" - ">=20\n" + "> \n" "> Ok, I'll change that.\n" - ">=20\n" - "> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the =\n" - "mask bits is protected by a\n" - "> spinlock. Elsewhere, you noted that the db bits are shared between =\n" - "all ports, so the db bitset is\n" - "> chopped up to be shared between the ports. Is the db mask also =\n" - "shared, and how is the spinlock\n" - "> sufficient for synchronizing access to the mask bits between multiple =\n" - "ports?\n" - ">=20\n" - "> Well, there are 64 doorbells that are shared between ports but each =\n" - "port\n" + "> \n" + "> > In switchtec_ntb_db_set_mask and friends, an in-memory copy of the mask bits is protected by a\n" + "> spinlock. Elsewhere, you noted that the db bits are shared between all ports, so the db bitset is\n" + "> chopped up to be shared between the ports. Is the db mask also shared, and how is the spinlock\n" + "> sufficient for synchronizing access to the mask bits between multiple ports?\n" + "> \n" + "> Well, there are 64 doorbells that are shared between ports but each port\n" "> has it's own in and out registers for the doorbells. So triggering\n" - "> doorbell one on one port's ODB actually triggers it on every ports =\n" - "IDB.\n" - "> So these are shared only in the sense that each port needs to know =\n" - "which\n" - "> dbs it cares about. Seeing each port has their own registers they =\n" - "don't\n" + "> doorbell one on one port's ODB actually triggers it on every ports IDB.\n" + "> So these are shared only in the sense that each port needs to know which\n" + "> dbs it cares about. Seeing each port has their own registers they don't\n" "> have to worry about synchronization.\n" - ">=20\n" + "> \n" "> The mask is only protected by a spin lock seeing multiple callers of\n" "> db_set_mask and db_clr_mask on the same port may step on each others\n" - "> toes. So if two processes try to mask different bits they both must =\n" - "get\n" + "> toes. So if two processes try to mask different bits they both must get\n" "> masked in the end and therefore some kind of synchronization must be\n" "> involved.\n" "\n" - "Thanks for clearing that up. Now I understand, each port has its own =\n" - "independent set of mask bits. So, while the doorbell numbers are =\n" - "assigned globally, the registers themselves are per port. For the mask =\n" - "bits, the mask behavior only affects the local port.\n" + "Thanks for clearing that up. Now I understand, each port has its own independent set of mask bits. So, while the doorbell numbers are assigned globally, the registers themselves are per port. For the mask bits, the mask behavior only affects the local port.\n" "\n" - ">=20\n" - "> > The IDT switch also does not have hardware scratchpads. Could the =\n" - "code you wrote for emulated\n" - "> scratchpads be made into shared library code for ntb drivers? Also, =\n" - "some ntb clients may not need\n" - "> scratchpad support. If it is not natively supported by a driver, can =\n" - "the emulated scratchpad support\n" + "> \n" + "> > The IDT switch also does not have hardware scratchpads. Could the code you wrote for emulated\n" + "> scratchpads be made into shared library code for ntb drivers? Also, some ntb clients may not need\n" + "> scratchpad support. If it is not natively supported by a driver, can the emulated scratchpad support\n" "> be an optional feature?\n" - ">=20\n" - "> Hmm, interesting idea. A few pieces could possibly be made common but =\n" - "it\n" + "> \n" + "> Hmm, interesting idea. A few pieces could possibly be made common but it\n" "> depends mostly on hardware having the resources to make use of it.\n" "> Switchtec has extra LUT memory windows that made this possible. Unless\n" - "> you object I'm inclined to leave it as is and I'd be happy to work =\n" - "with\n" + "> you object I'm inclined to leave it as is and I'd be happy to work with\n" "> the IDT folks to create a common solution in the future.\n" "\n" - "Alright. I'll leave it to you to find and reconcile common =\n" - "functionalities of the drivers. What about making spad emulation =\n" - "optional?\n" + "Alright. I'll leave it to you to find and reconcile common functionalities of the drivers. What about making spad emulation optional?\n" "\n" - ">=20\n" + "> \n" "> Logan\n" "\n" - "There was a comment on irc.oftc.net #ntb wishing for patch v2 to be =\n" - "fewer patches. Something like, 1/2: prep the existing switch driver, =\n" - 2/2: introduce the ntb driver. + There was a comment on irc.oftc.net #ntb wishing for patch v2 to be fewer patches. Something like, 1/2: prep the existing switch driver, 2/2: introduce the ntb driver. -d85b25c4777300a36a89e49ddc61047b62ab658858bbcff93fc4a72c29891f12 +98a73fac3edd826c15cc5e557e41c05a6d0ce9fcdeaa6bbb269ac0a65f98e02b
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.