From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: "Allen Hubbe" To: "'Logan Gunthorpe'" , , , Cc: "'Jon Mason'" , "'Dave Jiang'" , "'Bjorn Helgaas'" , "'Greg Kroah-Hartman'" , "'Kurt Schwemmer'" , "'Stephen Bates'" , "'Serge Semin'" , References: <20170615203729.9009-1-logang@deltatee.com> <000001d2e6a7$dfc719a0$9f554ce0$@dell.com> In-Reply-To: Subject: RE: [RFC PATCH 00/13] Switchtec NTB Support Date: Fri, 16 Jun 2017 11:34:34 -0400 Message-ID: <000101d2e6b6$0fcc2880$2f647980$@dell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: From: Logan Gunthorpe > On 16/06/17 07:53 AM, Allen Hubbe wrote: > > See what is staged in https://github.com/jonmason/ntb.git ntb-next, = with the addition of multi-peer > support by Serge. It would be good at this stage to understand = whether the api changes there would > also support the Switchtec driver, and what if anything must change, = or be planned to change, to > support the Switchtec driver. >=20 > Ah, yes I had seen that patchset some time ago but I wasn't aware of > it's status or that it was queued up in ntb-next. I think it will be = no > problem to reconcile with the switchtec driver and I'll rebase onto > ntb-next for the next posting of the patch set. However, I *may* save > full multi-host switchtec support for a follow up submission. My = initial > impression is the new API will support the switchtec hardware well. Alright! In code review, I really only have found minor nits. Overall, the = driver looks good. 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. There are a few instances like this: > + dev_dbg(&stdev->dev, "%s\n", __func__); Where the printing of __func__ could be controlled by dyndbg=3D+pf. The = debug message could be more useful. 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? 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 > Thanks, >=20 > Logan From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa8.dell-outbound.iphmx.com (esa8.dell-outbound.iphmx.com. [68.232.149.218]) by gmr-mx.google.com with ESMTPS id l135si155952ywb.10.2017.06.16.08.34.57 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 16 Jun 2017 08:34:57 -0700 (PDT) From: "Allen Hubbe" References: <20170615203729.9009-1-logang@deltatee.com> <000001d2e6a7$dfc719a0$9f554ce0$@dell.com> In-Reply-To: Subject: RE: [RFC PATCH 00/13] Switchtec NTB Support Date: Fri, 16 Jun 2017 11:34:34 -0400 Message-ID: <000101d2e6b6$0fcc2880$2f647980$@dell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Content-Language: en-us To: 'Logan Gunthorpe' , linux-ntb@googlegroups.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Cc: 'Jon Mason' , 'Dave Jiang' , 'Bjorn Helgaas' , 'Greg Kroah-Hartman' , 'Kurt Schwemmer' , 'Stephen Bates' , 'Serge Semin' , Sergey.Semin@t-platforms.ru List-ID: From: Logan Gunthorpe > On 16/06/17 07:53 AM, Allen Hubbe wrote: > > See what is staged in https://github.com/jonmason/ntb.git ntb-next, = with the addition of multi-peer > support by Serge. It would be good at this stage to understand = whether the api changes there would > also support the Switchtec driver, and what if anything must change, = or be planned to change, to > support the Switchtec driver. >=20 > Ah, yes I had seen that patchset some time ago but I wasn't aware of > it's status or that it was queued up in ntb-next. I think it will be = no > problem to reconcile with the switchtec driver and I'll rebase onto > ntb-next for the next posting of the patch set. However, I *may* save > full multi-host switchtec support for a follow up submission. My = initial > impression is the new API will support the switchtec hardware well. Alright! In code review, I really only have found minor nits. Overall, the = driver looks good. 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. There are a few instances like this: > + dev_dbg(&stdev->dev, "%s\n", __func__); Where the printing of __func__ could be controlled by dyndbg=3D+pf. The = debug message could be more useful. 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? 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 > Thanks, >=20 > Logan From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752108AbdFPPfK (ORCPT ); Fri, 16 Jun 2017 11:35:10 -0400 Received: from esa2.dell-outbound.iphmx.com ([68.232.149.220]:50843 "EHLO esa2.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751922AbdFPPfG (ORCPT ); Fri, 16 Jun 2017 11:35:06 -0400 X-DKIM: OpenDKIM Filter v2.4.3 mailuogwprd02.lss.emc.com v5GFYoBc004007 From: "Allen Hubbe" To: "'Logan Gunthorpe'" , , , Cc: "'Jon Mason'" , "'Dave Jiang'" , "'Bjorn Helgaas'" , "'Greg Kroah-Hartman'" , "'Kurt Schwemmer'" , "'Stephen Bates'" , "'Serge Semin'" , References: <20170615203729.9009-1-logang@deltatee.com> <000001d2e6a7$dfc719a0$9f554ce0$@dell.com> In-Reply-To: Subject: RE: [RFC PATCH 00/13] Switchtec NTB Support Date: Fri, 16 Jun 2017 11:34:34 -0400 Message-ID: <000101d2e6b6$0fcc2880$2f647980$@dell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" X-Mailer: Microsoft Outlook 15.0 Thread-Index: AQHS5hdH5H3YN4pAa0Woa+gX5poGvaInfWeQgABN7YD//82UsA== Content-Language: en-us X-RSA-Classifications: public X-Sentrion-Hostname: mailuogwprd02.lss.emc.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v5GFZEZl026247 From: Logan Gunthorpe > On 16/06/17 07:53 AM, Allen Hubbe wrote: > > See what is staged in https://github.com/jonmason/ntb.git ntb-next, with the addition of multi-peer > support by Serge. It would be good at this stage to understand whether the api changes there would > also support the Switchtec driver, and what if anything must change, or be planned to change, to > support the Switchtec driver. > > Ah, yes I had seen that patchset some time ago but I wasn't aware of > it's status or that it was queued up in ntb-next. I think it will be no > problem to reconcile with the switchtec driver and I'll rebase onto > ntb-next for the next posting of the patch set. However, I *may* save > full multi-host switchtec support for a follow up submission. My initial > impression is the new API will support the switchtec hardware well. Alright! In code review, I really only have found minor nits. Overall, the driver looks good. 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. There are a few instances like this: > + dev_dbg(&stdev->dev, "%s\n", __func__); Where the printing of __func__ could be controlled by dyndbg=+pf. The debug message could be more useful. 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? 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? > > Thanks, > > Logan