| Message ID | 20220830093207.951704-1-koba.ko@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
Hi, Koba Ko
Thanks for your patch.
I've had the same problem,see
https://lore.kernel.org/all/20220716024453.1418259-1-haijie1@huawei.com/.
The two operations of updating client_count, that is,
chan->client_count++;
balance_ref_count(chan);
the order of which is modified by d2f4f99db3e9 ("dmaengine: Rework
dma_chan_get").
I have complied and tested it on my arm64 and this patch does
fix the problem.
For this patch,
Reviewed-by: Jie Hai <haijie1@huawei.com>
Test-by: Jie Hai <haijie1@huawei.com>
Best regards,
Jie Hai
On 2022/8/30 17:32, Koba Ko wrote:
> If the passed client_count is 0,
> it would be incremented by balance_ref_count first
> then increment one more.
> This would cause client_count to 2.
>
> cat /sys/class/dma/dma0chan*/in_use
> 2
> 2
> 2
>
> Fixes: d2f4f99db3e9 ("dmaengine: Rework dma_chan_get")
> Signed-off-by: Koba Ko <koba.ko@canonical.com>
> ---
> drivers/dma/dmaengine.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 2cfa8458b51be..78f8a9f3ad825 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -451,7 +451,8 @@ static int dma_chan_get(struct dma_chan *chan)
> /* The channel is already in use, update client count */
> if (chan->client_count) {
> __module_get(owner);
> - goto out;
> + chan->client_count++;
> + return 0;
> }
>
> if (!try_module_get(owner))
> @@ -470,11 +471,11 @@ static int dma_chan_get(struct dma_chan *chan)
> goto err_out;
> }
>
> + chan->client_count++;
> +
> if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask))
> balance_ref_count(chan);
>
> -out:
> - chan->client_count++;
> return 0;
>
> err_out:
On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > If the passed client_count is 0, > it would be incremented by balance_ref_count first > then increment one more. > This would cause client_count to 2. > > cat /sys/class/dma/dma0chan*/in_use > 2 > 2 > 2 > > Fixes: d2f4f99db3e9 ("dmaengine: Rework dma_chan_get") > Signed-off-by: Koba Ko <koba.ko@canonical.com> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com > --- > drivers/dma/dmaengine.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 2cfa8458b51be..78f8a9f3ad825 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -451,7 +451,8 @@ static int dma_chan_get(struct dma_chan *chan) > /* The channel is already in use, update client count */ > if (chan->client_count) { > __module_get(owner); > - goto out; > + chan->client_count++; > + return 0; > } > > if (!try_module_get(owner)) > @@ -470,11 +471,11 @@ static int dma_chan_get(struct dma_chan *chan) > goto err_out; > } > > + chan->client_count++; > + > if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask)) > balance_ref_count(chan); > > -out: > - chan->client_count++; > return 0; > > err_out: > -- > 2.25.1 >
On 8/30/2022 2:32 AM, Koba Ko wrote: > If the passed client_count is 0, > it would be incremented by balance_ref_count first > then increment one more. > This would cause client_count to 2. > > cat /sys/class/dma/dma0chan*/in_use > 2 > 2 > 2 > > Fixes: d2f4f99db3e9 ("dmaengine: Rework dma_chan_get") > Signed-off-by: Koba Ko <koba.ko@canonical.com> Reviewed-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/dma/dmaengine.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 2cfa8458b51be..78f8a9f3ad825 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -451,7 +451,8 @@ static int dma_chan_get(struct dma_chan *chan) > /* The channel is already in use, update client count */ > if (chan->client_count) { > __module_get(owner); > - goto out; > + chan->client_count++; > + return 0; > } > > if (!try_module_get(owner)) > @@ -470,11 +471,11 @@ static int dma_chan_get(struct dma_chan *chan) > goto err_out; > } > > + chan->client_count++; > + > if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask)) > balance_ref_count(chan); > > -out: > - chan->client_count++; > return 0; > > err_out:
On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > If the passed client_count is 0, > it would be incremented by balance_ref_count first > then increment one more. > This would cause client_count to 2. > > cat /sys/class/dma/dma0chan*/in_use > 2 > 2 > 2 > > Fixes: d2f4f99db3e9 ("dmaengine: Rework dma_chan_get") > Signed-off-by: Koba Ko <koba.ko@canonical.com> > --- > drivers/dma/dmaengine.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c > index 2cfa8458b51be..78f8a9f3ad825 100644 > --- a/drivers/dma/dmaengine.c > +++ b/drivers/dma/dmaengine.c > @@ -451,7 +451,8 @@ static int dma_chan_get(struct dma_chan *chan) > /* The channel is already in use, update client count */ > if (chan->client_count) { > __module_get(owner); > - goto out; > + chan->client_count++; > + return 0; > } > > if (!try_module_get(owner)) > @@ -470,11 +471,11 @@ static int dma_chan_get(struct dma_chan *chan) > goto err_out; > } > > + chan->client_count++; > + > if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask)) > balance_ref_count(chan); > > -out: > - chan->client_count++; > return 0; > > err_out: > -- > 2.25.1 > Hi Vinod, Any thoughts on this patch? We recently came across this issue as well. Thanks, Jerry
On 29-09-22, 09:57, Jerry Snitselaar wrote: > On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > > Hi Vinod, > > Any thoughts on this patch? We recently came across this issue as well. I have only patch 3, where is the rest of the series... ?
On Thu, 2022-09-29 at 22:40 +0530, Vinod Koul wrote: > On 29-09-22, 09:57, Jerry Snitselaar wrote: > > On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > > > > > Hi Vinod, > > > > Any thoughts on this patch? We recently came across this issue as > > well. > > I have only patch 3, where is the rest of the series... ? > I never found anything else when I looked at this earlier. The one thing I can think of is perhaps Koba was seeing multiple issues at time when he found this, like: https://lore.kernel.org/linux-crypto/20220901144712.1192698-1-koba.ko@canonical.com/ That was also being seen by an engineer here that was looking at client_count code. Koba, was this meant to be part of a series, or by itself? Regards, Jerry
On Fri, Sep 30, 2022 at 1:26 AM Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > On Thu, 2022-09-29 at 22:40 +0530, Vinod Koul wrote: > > On 29-09-22, 09:57, Jerry Snitselaar wrote: > > > On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > > > > > > > > Hi Vinod, > > > > > > Any thoughts on this patch? We recently came across this issue as > > > well. > > > > I have only patch 3, where is the rest of the series... ? > > > > I never found anything else when I looked at this earlier. > The one thing I can think of is perhaps Koba was seeing multiple > issues at time when he found this, like: > > https://lore.kernel.org/linux-crypto/20220901144712.1192698-1-koba.ko@canonical.com/ > > That was also being seen by an engineer here that was looking > at client_count code. > > Koba, was this meant to be part of a series, or by itself? > Jerry, you're right, it's a part of the series. > > Regards, > Jerry >
On Fri, Sep 30, 2022 at 12:44:22PM +0800, Koba Ko wrote: > On Fri, Sep 30, 2022 at 1:26 AM Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > > > On Thu, 2022-09-29 at 22:40 +0530, Vinod Koul wrote: > > > On 29-09-22, 09:57, Jerry Snitselaar wrote: > > > > On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > > > > > > > > > > > Hi Vinod, > > > > > > > > Any thoughts on this patch? We recently came across this issue as > > > > well. > > > > > > I have only patch 3, where is the rest of the series... ? > > > > > > > I never found anything else when I looked at this earlier. > > The one thing I can think of is perhaps Koba was seeing multiple > > issues at time when he found this, like: > > > > https://lore.kernel.org/linux-crypto/20220901144712.1192698-1-koba.ko@canonical.com/ > > > > That was also being seen by an engineer here that was looking > > at client_count code. > > > > Koba, was this meant to be part of a series, or by itself? > > > > Jerry, you're right, it's a part of the series. Hi Koba, If it is meant to be part of a series, where are patches 1 and 2? The ccp patch has already been applied by the crypto maintainers if that was meant to be part of a series with this patch. Regards, Jerry > > > > > Regards, > > Jerry > >
On Fri, Sep 30, 2022 at 1:56 PM Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > On Fri, Sep 30, 2022 at 12:44:22PM +0800, Koba Ko wrote: > > On Fri, Sep 30, 2022 at 1:26 AM Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > > > > > On Thu, 2022-09-29 at 22:40 +0530, Vinod Koul wrote: > > > > On 29-09-22, 09:57, Jerry Snitselaar wrote: > > > > > On Tue, Aug 30, 2022 at 05:32:07PM +0800, Koba Ko wrote: > > > > > > > > > > > > > > Hi Vinod, > > > > > > > > > > Any thoughts on this patch? We recently came across this issue as > > > > > well. > > > > > > > > I have only patch 3, where is the rest of the series... ? > > > > > > > > > > I never found anything else when I looked at this earlier. > > > The one thing I can think of is perhaps Koba was seeing multiple > > > issues at time when he found this, like: > > > > > > https://lore.kernel.org/linux-crypto/20220901144712.1192698-1-koba.ko@canonical.com/ > > > > > > That was also being seen by an engineer here that was looking > > > at client_count code. > > > > > > Koba, was this meant to be part of a series, or by itself? > > > > > > > Jerry, you're right, it's a part of the series. > > Hi Koba, > > If it is meant to be part of a series, where are patches 1 and 2? > The ccp patch has already been applied by the crypto maintainers if that > was meant to be part of a series with this patch. > > Regards, > Jerry Sorry, I misunderstood. actually, there's a mistake on the [3/3] part. I created patches for the mainline kernel before sending them to the upstream. Then I found the first has existed on the patchwork, so removed the [2/3] part for ccp patch and forgot to modify for this. Should I fix this and re-submit v2(also add those review-by)? > > > > > > > > > Regards, > > > Jerry > > > >
On 30-09-22, 14:25, Koba Ko wrote: > On Fri, Sep 30, 2022 at 1:56 PM Jerry Snitselaar <jsnitsel@redhat.com> wrote: > > Sorry, I misunderstood. actually, there's a mistake on the [3/3] part. > I created patches for the mainline kernel before sending them to the upstream. > Then I found the first has existed on the patchwork, so removed the > [2/3] part for ccp patch and forgot to modify for this. > Should I fix this and re-submit v2(also add those review-by)? Yes please
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index 2cfa8458b51be..78f8a9f3ad825 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -451,7 +451,8 @@ static int dma_chan_get(struct dma_chan *chan) /* The channel is already in use, update client count */ if (chan->client_count) { __module_get(owner); - goto out; + chan->client_count++; + return 0; } if (!try_module_get(owner)) @@ -470,11 +471,11 @@ static int dma_chan_get(struct dma_chan *chan) goto err_out; } + chan->client_count++; + if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask)) balance_ref_count(chan); -out: - chan->client_count++; return 0; err_out:
If the passed client_count is 0, it would be incremented by balance_ref_count first then increment one more. This would cause client_count to 2. cat /sys/class/dma/dma0chan*/in_use 2 2 2 Fixes: d2f4f99db3e9 ("dmaengine: Rework dma_chan_get") Signed-off-by: Koba Ko <koba.ko@canonical.com> --- drivers/dma/dmaengine.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)