Message ID | 20250121091241.2646532-1-quic_mdalam@quicinc.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [v3] dmaengine: qcom: bam_dma: Fix BAM_RIVISON register handling | expand |
On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote: > This patch resolves a bug from the previous commit where the Please check Documentation/process/submitting-patches.rst, 'This patch...' > BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP > mode. The issue was reading the BAM_REVISION register hanging if num-ees First start with the issue description, then proceed to the changes description. > was not zero, which occurs when the SoCs power on BAM remotely. So the > BAM_REVISION register read has been moved to inside if condition. Imperative language, please. While we are at it, please also fix commit subject. > > Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register") > Reported-by: Georgi Djakov <djakov@kernel.org> > Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/ > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > > Change in [v3] > > * Revised commit details > > Change in [v2] > > * Removed unnecessary if checks. > * Relocated the BAM_REVISION register read within the if condition. > > Change in [v1] > > * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/ > * Posted initial fixup for BAM revision register read handling > drivers/dma/qcom/bam_dma.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c14557efd577..d227b4f5b6b9 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev) > u32 val; > > /* read revision and configuration information */ Please extend the comment, mentioning why the register is read only in !num_ees case. BTW: how do we get revision if num_ees != 0? > - val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); > - if (!bdev->num_ees) > + if (!bdev->num_ees) { > + val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); > bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK; > - > - bdev->bam_revision = val & REVISION_MASK; > + bdev->bam_revision = val & REVISION_MASK; > + } > > /* check that configured EE is within range */ > if (bdev->ee >= bdev->num_ees) > -- > 2.34.1 >
On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote: > This patch resolves a bug from the previous commit where the > BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP > mode. The issue was reading the BAM_REVISION register hanging if num-ees > was not zero, which occurs when the SoCs power on BAM remotely. So the > BAM_REVISION register read has been moved to inside if condition. > > Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register") > Reported-by: Georgi Djakov <djakov@kernel.org> > Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/ > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> I'm afraid there are still two open problems here: 1. In your original commit, you added the if (in_range(...)) checks to make the BAM_DESC_CNT_TRSHLD register write conditional. With this patch we only read the bam_revision for the !bdev->num_ees case. This means that even if we have e.g. a remotely powered BAM-NDP, we don't initialize BAM_DESC_CNT_TRSHLD anymore. 2. Aside from BAM-NDP and BAM-Lite there is also plain "BAM". You mentioned we should only skip the register write for BAM-Lite, but the plain "BAM" isn't handled anywhere yet. I would recommend inverting the in_range(...) checks to check for if (!in_range(BAM-LITE) rather than if (in_range(BAM-NDP)). This should also work for the plain "BAM" type. It will also avoid regressions if we don't read the bam_revision in the !bdev->num_ees case. (Although ideally you would lazily initialize the bam_revision to cover all the configurations.) Thanks, Stephan > --- > > Change in [v3] > > * Revised commit details > > Change in [v2] > > * Removed unnecessary if checks. > * Relocated the BAM_REVISION register read within the if condition. > > Change in [v1] > > * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/ > * Posted initial fixup for BAM revision register read handling > drivers/dma/qcom/bam_dma.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c > index c14557efd577..d227b4f5b6b9 100644 > --- a/drivers/dma/qcom/bam_dma.c > +++ b/drivers/dma/qcom/bam_dma.c > @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev) > u32 val; > > /* read revision and configuration information */ > - val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); > - if (!bdev->num_ees) > + if (!bdev->num_ees) { > + val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); > bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK; > - > - bdev->bam_revision = val & REVISION_MASK; > + bdev->bam_revision = val & REVISION_MASK; > + } > > /* check that configured EE is within range */ > if (bdev->ee >= bdev->num_ees) > -- > 2.34.1 >
On 1/21/2025 4:24 PM, Dmitry Baryshkov wrote: > On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote: >> This patch resolves a bug from the previous commit where the > > Please check Documentation/process/submitting-patches.rst, 'This > patch...' Ok > >> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP >> mode. The issue was reading the BAM_REVISION register hanging if num-ees > > First start with the issue description, then proceed to the changes > description. Ok > >> was not zero, which occurs when the SoCs power on BAM remotely. So the >> BAM_REVISION register read has been moved to inside if condition. > > Imperative language, please. While we are at it, please also fix commit > subject. Sure , will fix this in next revision. > >> >> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register") >> Reported-by: Georgi Djakov <djakov@kernel.org> >> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/ >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> >> Change in [v3] >> >> * Revised commit details >> >> Change in [v2] >> >> * Removed unnecessary if checks. >> * Relocated the BAM_REVISION register read within the if condition. >> >> Change in [v1] >> >> * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/ >> * Posted initial fixup for BAM revision register read handling >> drivers/dma/qcom/bam_dma.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c >> index c14557efd577..d227b4f5b6b9 100644 >> --- a/drivers/dma/qcom/bam_dma.c >> +++ b/drivers/dma/qcom/bam_dma.c >> @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev) >> u32 val; >> >> /* read revision and configuration information */ > > Please extend the comment, mentioning why the register is read only in > !num_ees case. BTW: how do we get revision if num_ees != 0? This revision register we need only to differentiate b/w BAM-Lite and BAM-NDP. if num_ees != 0 then no need read this revision register. The SOCs which is having BAM-NDP and BAM-Lite having num_ees = 0. > >> - val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); >> - if (!bdev->num_ees) >> + if (!bdev->num_ees) { >> + val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); >> bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK; >> - >> - bdev->bam_revision = val & REVISION_MASK; >> + bdev->bam_revision = val & REVISION_MASK; >> + } >> >> /* check that configured EE is within range */ >> if (bdev->ee >= bdev->num_ees) >> -- >> 2.34.1 >> >
On 1/21/2025 10:39 PM, Stephan Gerhold wrote: > On Tue, Jan 21, 2025 at 02:42:41PM +0530, Md Sadre Alam wrote: >> This patch resolves a bug from the previous commit where the >> BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP >> mode. The issue was reading the BAM_REVISION register hanging if num-ees >> was not zero, which occurs when the SoCs power on BAM remotely. So the >> BAM_REVISION register read has been moved to inside if condition. >> >> Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register") >> Reported-by: Georgi Djakov <djakov@kernel.org> >> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/ >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > > I'm afraid there are still two open problems here: > > 1. In your original commit, you added the if (in_range(...)) checks to > make the BAM_DESC_CNT_TRSHLD register write conditional. With this > patch we only read the bam_revision for the !bdev->num_ees case. > This means that even if we have e.g. a remotely powered BAM-NDP, > we don't initialize BAM_DESC_CNT_TRSHLD anymore. > > 2. Aside from BAM-NDP and BAM-Lite there is also plain "BAM". You > mentioned we should only skip the register write for BAM-Lite, but > the plain "BAM" isn't handled anywhere yet. > > I would recommend inverting the in_range(...) checks to check for if > (!in_range(BAM-LITE) rather than if (in_range(BAM-NDP)). This should > also work for the plain "BAM" type. It will also avoid regressions if we > don't read the bam_revision in the !bdev->num_ees case. (Although > ideally you would lazily initialize the bam_revision to cover all the > configurations.) Thanks for explanation and suggestion. Will address all the above points in next revision. > > Thanks, > Stephan > >> --- >> >> Change in [v3] >> >> * Revised commit details >> >> Change in [v2] >> >> * Removed unnecessary if checks. >> * Relocated the BAM_REVISION register read within the if condition. >> >> Change in [v1] >> >> * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/ >> * Posted initial fixup for BAM revision register read handling >> drivers/dma/qcom/bam_dma.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c >> index c14557efd577..d227b4f5b6b9 100644 >> --- a/drivers/dma/qcom/bam_dma.c >> +++ b/drivers/dma/qcom/bam_dma.c >> @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev) >> u32 val; >> >> /* read revision and configuration information */ >> - val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); >> - if (!bdev->num_ees) >> + if (!bdev->num_ees) { >> + val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); >> bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK; >> - >> - bdev->bam_revision = val & REVISION_MASK; >> + bdev->bam_revision = val & REVISION_MASK; >> + } >> >> /* check that configured EE is within range */ >> if (bdev->ee >= bdev->num_ees) >> -- >> 2.34.1 >>
diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c index c14557efd577..d227b4f5b6b9 100644 --- a/drivers/dma/qcom/bam_dma.c +++ b/drivers/dma/qcom/bam_dma.c @@ -1199,11 +1199,11 @@ static int bam_init(struct bam_device *bdev) u32 val; /* read revision and configuration information */ - val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); - if (!bdev->num_ees) + if (!bdev->num_ees) { + val = readl_relaxed(bam_addr(bdev, 0, BAM_REVISION)); bdev->num_ees = (val >> NUM_EES_SHIFT) & NUM_EES_MASK; - - bdev->bam_revision = val & REVISION_MASK; + bdev->bam_revision = val & REVISION_MASK; + } /* check that configured EE is within range */ if (bdev->ee >= bdev->num_ees)
This patch resolves a bug from the previous commit where the BAM_DESC_CNT_TRSHLD register was conditionally written based on BAM-NDP mode. The issue was reading the BAM_REVISION register hanging if num-ees was not zero, which occurs when the SoCs power on BAM remotely. So the BAM_REVISION register read has been moved to inside if condition. Fixes: 57a7138d0627 ("dmaengine: qcom: bam_dma: Avoid writing unavailable register") Reported-by: Georgi Djakov <djakov@kernel.org> Link: https://lore.kernel.org/lkml/9ef3daa8-cdb1-49f2-8d19-a72d6210ff3a@kernel.org/ Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> --- Change in [v3] * Revised commit details Change in [v2] * Removed unnecessary if checks. * Relocated the BAM_REVISION register read within the if condition. Change in [v1] * https://lore.kernel.org/lkml/1a5fc7e9-39fe-e527-efc3-1ea990bbb53b@quicinc.com/ * Posted initial fixup for BAM revision register read handling drivers/dma/qcom/bam_dma.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)