* [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads @ 2016-12-01 19:11 Shyam Sundar S K 2016-12-02 2:22 ` Allen Hubbe 0 siblings, 1 reply; 6+ messages in thread From: Shyam Sundar S K @ 2016-12-01 19:11 UTC (permalink / raw) To: Jon Mason, Allen Hubbe, Dave Jiang Cc: Yu, Xiangliang, Shah, Nehal-bakulchandra, Agrawal, Nitesh-kumar, Sen, Pankaj, Su, Richard (Bin), Subramaniyan, Ramkumar, linux-ntb When the underlying NTB H/W driver advertises more memory windows than the number of scratchpads available to setup MW's, it is likely that we may end up filling the remaining memory windows with garbage. So to avoid that, lets limit the memory windows that transport driver can setup based on the available scratchpads. Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com> Reviewed-by: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> --- drivers/ntb/ntb_transport.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c index 4eb8adb..50d6b06 100644 --- a/drivers/ntb/ntb_transport.c +++ b/drivers/ntb/ntb_transport.c @@ -1064,7 +1064,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev) { struct ntb_transport_ctx *nt; struct ntb_transport_mw *mw; - unsigned int mw_count, qp_count; + unsigned int mw_count, qp_count, spad_count, max_mw_count_for_spads; u64 qp_bitmap; int node; int rc, i; @@ -1090,8 +1090,16 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev) return -ENOMEM; nt->ndev = ndev; + spad_count = ntb_spad_count(ndev); - nt->mw_count = mw_count; + /* Limit the MW's based on the availability of scratchpads */ + if (spad_count > NUM_MWS + 2) { + max_mw_count_for_spads = (spad_count - (NUM_MWS + 1)) >> 1; + nt->mw_count = min(mw_count, max_mw_count_for_spads); + } else { + nt->mw_count = 0; + goto err; + } nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec), GFP_KERNEL, node); -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads 2016-12-01 19:11 [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads Shyam Sundar S K @ 2016-12-02 2:22 ` Allen Hubbe 2016-12-02 12:32 ` Shyam Sundar S K 0 siblings, 1 reply; 6+ messages in thread From: Allen Hubbe @ 2016-12-02 2:22 UTC (permalink / raw) To: 'Shyam Sundar S K', 'Jon Mason', 'Dave Jiang' Cc: 'Yu, Xiangliang', 'Shah, Nehal-bakulchandra', 'Agrawal, Nitesh-kumar', 'Sen, Pankaj', 'Su, Richard (Bin)', 'Subramaniyan, Ramkumar', linux-ntb From: Shyam Sundar S K > When the underlying NTB H/W driver advertises more memory windows > than the number of scratchpads available to setup MW's, it is likely > that we may end up filling the remaining memory windows with garbage. > So to avoid that, lets limit the memory windows that transport driver > can setup based on the available scratchpads. This change should also touch: static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt) { ... /* The scratchpad registers keep the values if the remote side * goes down, blast them now to give them a sane value the next * time they are accessed */ for (i = 0; i < MAX_SPAD; i++) ntb_spad_write(nt->ndev, i, 0); The MAX_SPAD value is incorrect with three MWs and I think we should drop it. Instead, this section should ntb_spad_write(ndev, i, 0) for i in 0..ntb_spad_count(ndev). Not having exactly two memory windows, these may be dropped as well: MW1_SZ_HIGH, MW1_SZ_LOW. They seem to be unused in the code, anyway. It is still useful to keep MW0_SZ_HIGH and MW0_SZ_LOW for indexing. > > Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com> > Reviewed-by: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com> > Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> > --- > drivers/ntb/ntb_transport.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c > index 4eb8adb..50d6b06 100644 > --- a/drivers/ntb/ntb_transport.c > +++ b/drivers/ntb/ntb_transport.c > @@ -1064,7 +1064,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct > ntb_dev *ndev) > { > struct ntb_transport_ctx *nt; > struct ntb_transport_mw *mw; > - unsigned int mw_count, qp_count; > + unsigned int mw_count, qp_count, spad_count, max_mw_count_for_spads; > u64 qp_bitmap; > int node; > int rc, i; > @@ -1090,8 +1090,16 @@ static int ntb_transport_probe(struct ntb_client *self, struct > ntb_dev *ndev) > return -ENOMEM; > > nt->ndev = ndev; > + spad_count = ntb_spad_count(ndev); > > - nt->mw_count = mw_count; > + /* Limit the MW's based on the availability of scratchpads */ > + if (spad_count > NUM_MWS + 2) { > + max_mw_count_for_spads = (spad_count - (NUM_MWS + 1)) >> 1; > + nt->mw_count = min(mw_count, max_mw_count_for_spads); > + } else { > + nt->mw_count = 0; > + goto err; > + } It is somewhat confusing to have a NUM_MWS + 2 and also NUM_MWS + 1. We could #define the minimum number of spads needed by ntb_transport. #define NTB_TRANSPORT_MIN_SPADS (MW0_SZ_HIGH + 2) I think it would be more clearly written as: if (spad_count < NTB_TRANSPORT_MIN_SPADS) { nt->mw_count = 0; goto err; } max_mw_count_for_spads = (spad_count - MW0_SZ_HIGH) / 2; nt->mw_count = min(mw_count, max_mw_count_for_spads); > > nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec), > GFP_KERNEL, node); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads 2016-12-02 2:22 ` Allen Hubbe @ 2016-12-02 12:32 ` Shyam Sundar S K 2016-12-02 14:30 ` Allen Hubbe 0 siblings, 1 reply; 6+ messages in thread From: Shyam Sundar S K @ 2016-12-02 12:32 UTC (permalink / raw) To: Allen Hubbe, 'Jon Mason', 'Dave Jiang' Cc: 'Yu, Xiangliang', 'Shah, Nehal-bakulchandra', 'Agrawal, Nitesh-kumar', 'Sen, Pankaj', 'Su, Richard (Bin)', 'Subramaniyan, Ramkumar', linux-ntb On 12/2/2016 7:52 AM, Allen Hubbe wrote: > From: Shyam Sundar S K >> When the underlying NTB H/W driver advertises more memory windows >> than the number of scratchpads available to setup MW's, it is likely >> that we may end up filling the remaining memory windows with garbage. >> So to avoid that, lets limit the memory windows that transport driver >> can setup based on the available scratchpads. > > This change should also touch: > > static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt) > { > ... > /* The scratchpad registers keep the values if the remote side > * goes down, blast them now to give them a sane value the next > * time they are accessed > */ > for (i = 0; i < MAX_SPAD; i++) > ntb_spad_write(nt->ndev, i, 0); > > The MAX_SPAD value is incorrect with three MWs and I think we should drop it. Instead, this section should ntb_spad_write(ndev, i, 0) for i in 0..ntb_spad_count(ndev). > > Not having exactly two memory windows, these may be dropped as well: MW1_SZ_HIGH, MW1_SZ_LOW. They seem to be unused in the code, anyway. > > It is still useful to keep MW0_SZ_HIGH and MW0_SZ_LOW for indexing. > >> >> Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com> >> Reviewed-by: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com> >> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> >> --- >> drivers/ntb/ntb_transport.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c >> index 4eb8adb..50d6b06 100644 >> --- a/drivers/ntb/ntb_transport.c >> +++ b/drivers/ntb/ntb_transport.c >> @@ -1064,7 +1064,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct >> ntb_dev *ndev) >> { >> struct ntb_transport_ctx *nt; >> struct ntb_transport_mw *mw; >> - unsigned int mw_count, qp_count; >> + unsigned int mw_count, qp_count, spad_count, max_mw_count_for_spads; >> u64 qp_bitmap; >> int node; >> int rc, i; >> @@ -1090,8 +1090,16 @@ static int ntb_transport_probe(struct ntb_client *self, struct >> ntb_dev *ndev) >> return -ENOMEM; >> >> nt->ndev = ndev; >> + spad_count = ntb_spad_count(ndev); >> >> - nt->mw_count = mw_count; >> + /* Limit the MW's based on the availability of scratchpads */ >> + if (spad_count > NUM_MWS + 2) { >> + max_mw_count_for_spads = (spad_count - (NUM_MWS + 1)) >> 1; >> + nt->mw_count = min(mw_count, max_mw_count_for_spads); >> + } else { >> + nt->mw_count = 0; >> + goto err; >> + } > > It is somewhat confusing to have a NUM_MWS + 2 and also NUM_MWS + 1. We could #define the minimum number of spads needed by ntb_transport. > > #define NTB_TRANSPORT_MIN_SPADS (MW0_SZ_HIGH + 2) > > I think it would be more clearly written as: > > if (spad_count < NTB_TRANSPORT_MIN_SPADS) { > nt->mw_count = 0; > goto err; > } > Allen, I will submit the patch which will accommodate all the suggestions you have made. Do you feel this part of the code is required in ntb_transport_probe() ? if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count * 2)) { dev_err(&ndev->dev, "Not enough scratch pad registers for %s", NTB_TRANSPORT_NAME); return -EIO; } Because in case of AMD, this condition will always fail. > max_mw_count_for_spads = (spad_count - MW0_SZ_HIGH) / 2; > nt->mw_count = min(mw_count, max_mw_count_for_spads); > >> >> nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec), >> GFP_KERNEL, node); >> -- >> 2.7.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads 2016-12-02 12:32 ` Shyam Sundar S K @ 2016-12-02 14:30 ` Allen Hubbe 2016-12-02 16:31 ` Shyam Sundar S K 0 siblings, 1 reply; 6+ messages in thread From: Allen Hubbe @ 2016-12-02 14:30 UTC (permalink / raw) To: 'Shyam Sundar S K', 'Jon Mason', 'Dave Jiang' Cc: 'Yu, Xiangliang', 'Shah, Nehal-bakulchandra', 'Agrawal, Nitesh-kumar', 'Sen, Pankaj', 'Su, Richard (Bin)', 'Subramaniyan, Ramkumar', linux-ntb From: Shyam Sundar S K > Do you feel this part of the code is required in ntb_transport_probe() ? > > if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count * 2)) { > dev_err(&ndev->dev, "Not enough scratch pad registers for %s", > NTB_TRANSPORT_NAME); > return -EIO; > } > > Because in case of AMD, this condition will always fail. You're right. That check can be removed with the addition of your other changes. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads 2016-12-02 14:30 ` Allen Hubbe @ 2016-12-02 16:31 ` Shyam Sundar S K 2016-12-02 16:57 ` Allen Hubbe 0 siblings, 1 reply; 6+ messages in thread From: Shyam Sundar S K @ 2016-12-02 16:31 UTC (permalink / raw) To: Allen Hubbe, 'Jon Mason', 'Dave Jiang' Cc: 'Yu, Xiangliang', 'Shah, Nehal-bakulchandra', 'Agrawal, Nitesh-kumar', 'Sen, Pankaj', 'Su, Richard (Bin)', 'Subramaniyan, Ramkumar', linux-ntb When the underlying NTB H/W driver advertises more memory windows than the number of scratchpads available to setup MW's, it is likely that we may end up filling the remaining memory windows with garbage. So to avoid that, lets limit the memory windows that transport driver can setup based on the available scratchpads. Reviewed-by: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com> Reviewed-by: Agrawal, Nitesh-kumar <Nitesh-kumar.Agrawal@amd.com> Signed-off-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> --- drivers/ntb/ntb_transport.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c index 4eb8adb..e02ad4e 100644 --- a/drivers/ntb/ntb_transport.c +++ b/drivers/ntb/ntb_transport.c @@ -66,6 +66,7 @@ #define NTB_TRANSPORT_VER "4" #define NTB_TRANSPORT_NAME "ntb_transport" #define NTB_TRANSPORT_DESC "Software Queue-Pair Transport over NTB" +#define NTB_TRANSPORT_MIN_SPADS (MW0_SZ_HIGH + 2) MODULE_DESCRIPTION(NTB_TRANSPORT_DESC); MODULE_VERSION(NTB_TRANSPORT_VER); @@ -242,9 +243,6 @@ enum { NUM_MWS, MW0_SZ_HIGH, MW0_SZ_LOW, - MW1_SZ_HIGH, - MW1_SZ_LOW, - MAX_SPAD, }; #define dev_client_dev(__dev) \ @@ -831,7 +829,7 @@ static void ntb_transport_link_cleanup(struct ntb_transport_ctx *nt) * goes down, blast them now to give them a sane value the next * time they are accessed */ - for (i = 0; i < MAX_SPAD; i++) + for (i = 0; i < ntb_spad_count(nt->ndev); i++) ntb_spad_write(nt->ndev, i, 0); } @@ -1064,17 +1062,12 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev) { struct ntb_transport_ctx *nt; struct ntb_transport_mw *mw; - unsigned int mw_count, qp_count; + unsigned int mw_count, qp_count, spad_count, max_mw_count_for_spads; u64 qp_bitmap; int node; int rc, i; mw_count = ntb_mw_count(ndev); - if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count * 2)) { - dev_err(&ndev->dev, "Not enough scratch pad registers for %s", - NTB_TRANSPORT_NAME); - return -EIO; - } if (ntb_db_is_unsafe(ndev)) dev_dbg(&ndev->dev, @@ -1090,8 +1083,17 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev) return -ENOMEM; nt->ndev = ndev; + spad_count = ntb_spad_count(ndev); + + /* Limit the MW's based on the availability of scratchpads */ + + if (spad_count < NTB_TRANSPORT_MIN_SPADS) { + nt->mw_count = 0; + goto err; + } - nt->mw_count = mw_count; + max_mw_count_for_spads = (spad_count - MW0_SZ_HIGH) / 2; + nt->mw_count = min(mw_count, max_mw_count_for_spads); nt->mw_vec = kzalloc_node(mw_count * sizeof(*nt->mw_vec), GFP_KERNEL, node); -- 2.7.4 On 12/2/2016 8:00 PM, Allen Hubbe wrote: > From: Shyam Sundar S K >> Do you feel this part of the code is required in ntb_transport_probe() ? >> >> if (ntb_spad_count(ndev) < (NUM_MWS + 1 + mw_count * 2)) { >> dev_err(&ndev->dev, "Not enough scratch pad registers for %s", >> NTB_TRANSPORT_NAME); >> return -EIO; >> } >> >> Because in case of AMD, this condition will always fail. > > You're right. That check can be removed with the addition of your other changes. > ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads 2016-12-02 16:31 ` Shyam Sundar S K @ 2016-12-02 16:57 ` Allen Hubbe 0 siblings, 0 replies; 6+ messages in thread From: Allen Hubbe @ 2016-12-02 16:57 UTC (permalink / raw) To: 'Shyam Sundar S K', 'Jon Mason', 'Dave Jiang' Cc: 'Yu, Xiangliang', 'Shah, Nehal-bakulchandra', 'Agrawal, Nitesh-kumar', 'Sen, Pankaj', 'Su, Richard (Bin)', 'Subramaniyan, Ramkumar', linux-ntb This should be subject: [PATCH v2 ...] Next should be PATCH v3. git format-patch -v3 From: Shyam Sundar S K > - for (i = 0; i < MAX_SPAD; i++) > + for (i = 0; i < ntb_spad_count(nt->ndev); i++) count = ntb_spad_count(nt->ndev); for (i = 0; i < count; i++) ... The rest looks good. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-02 16:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-01 19:11 [PATCH 1/2] ntb_transport: Limit memory windows based on available, scratchpads Shyam Sundar S K 2016-12-02 2:22 ` Allen Hubbe 2016-12-02 12:32 ` Shyam Sundar S K 2016-12-02 14:30 ` Allen Hubbe 2016-12-02 16:31 ` Shyam Sundar S K 2016-12-02 16:57 ` Allen Hubbe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox