[RFC PATCH 0/3] cxl: Fix background operation handling

[RFC PATCH 0/3] cxl: Fix background operation handling

[Jeongtae Park]

Hello,

We are currently trying to add background commands like
fw-related and sanitize operation. However, as we modified 
the code, we found that there was a lot of duplication of
code, so we would like to modify the existing background 
command handling a little bit. We want to make the most of
the existing background command handling so that we can
easily add other background commands in the future.

The first patch modifies the existing sanitize operation
handling to allow other background commands to be used 
with it, and the second patch modifies sanitize to work
with the modified code. Finally, the third patch adds a
sysfs file related to the background operation. 
This sysfs entry is select/poll capable from userspace 
to notify upon completion of a background operation.

Thanks,
/jtpark

Jeongtae Park (3):
  cxl: Modify background cmd handling for the others
  cxl/mbox: Update sanitize opcode support
  cxl/memdev: Add background command sysfs file

 Documentation/ABI/testing/sysfs-bus-cxl | 10 ++++
 drivers/cxl/core/mbox.c                 |  5 +-
 drivers/cxl/core/memdev.c               | 71 +++++++++++++++++++++++--
 drivers/cxl/cxlmem.h                    | 33 ++++++++++--
 drivers/cxl/pci.c                       | 65 +++++++++++-----------
 5 files changed, 141 insertions(+), 43 deletions(-)


base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
-- 
2.34.1

Re: [RFC PATCH 0/3] cxl: Fix background operation handling

[Dave Jiang]

On 9/22/23 06:05, Jeongtae Park wrote:
> Hello,
> 
> We are currently trying to add background commands like
> fw-related and sanitize operation. However, as we modified 
> the code, we found that there was a lot of duplication of
> code, so we would like to modify the existing background 
> command handling a little bit. We want to make the most of
> the existing background command handling so that we can
> easily add other background commands in the future.
> 
> The first patch modifies the existing sanitize operation
> handling to allow other background commands to be used 
> with it, and the second patch modifies sanitize to work
> with the modified code. Finally, the third patch adds a
> sysfs file related to the background operation. 
> This sysfs entry is select/poll capable from userspace 
> to notify upon completion of a background operation.
> 
> Thanks,
> /jtpark

Hi Jeongtae,
For the next version of the series you send out, can you please make sure the series is threaded? Currently all your series are coming as individual mails. Thanks!


> 
> Jeongtae Park (3):
>   cxl: Modify background cmd handling for the others
>   cxl/mbox: Update sanitize opcode support
>   cxl/memdev: Add background command sysfs file
> 
>  Documentation/ABI/testing/sysfs-bus-cxl | 10 ++++
>  drivers/cxl/core/mbox.c                 |  5 +-
>  drivers/cxl/core/memdev.c               | 71 +++++++++++++++++++++++--
>  drivers/cxl/cxlmem.h                    | 33 ++++++++++--
>  drivers/cxl/pci.c                       | 65 +++++++++++-----------
>  5 files changed, 141 insertions(+), 43 deletions(-)
> 
> 
> base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9

Re: [RFC PATCH 0/3] cxl: Fix background operation handling

[Davidlohr Bueso]

On Fri, 22 Sep 2023, Jeongtae Park wrote:

>Hello,
>
>We are currently trying to add background commands like
>fw-related and sanitize operation. However, as we modified
>the code, we found that there was a lot of duplication of
>code, so we would like to modify the existing background
>command handling a little bit. We want to make the most of
>the existing background command handling so that we can
>easily add other background commands in the future.

What code duplication? All you need to do is arm the
struct cxl_mbox_cmd with sensible timeouts. For example,
see 9521875bbe0 (cxl: add a firmware update mechanism using
the sysfs firmware loader).

>The first patch modifies the existing sanitize operation
>handling to allow other background commands to be used
>with it, and the second patch modifies sanitize to work
>with the modified code. Finally, the third patch adds a
>sysfs file related to the background operation.
>This sysfs entry is select/poll capable from userspace
>to notify upon completion of a background operation.

This says nothing about the motivation of these changes.
Specially when it has been well discussed in the past that
exactly because of its monopolizing nature, the driver will
_only_ do sanitize asynchronously. See the changelog of
ccadf1310fb (cxl/mbox: Add background cmd handling machinery).

Thanks,
Davidlohr

Re: [RFC PATCH 0/3] cxl: Fix background operation handling

[Jeongtae Park]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On Mon, Sep 25, 2023 at 03:56:54PM -0700, Dave Jiang wrote:
> 
> 
> On 9/22/23 06:05, Jeongtae Park wrote:
> > Hello,
> > 
> > We are currently trying to add background commands like
> > fw-related and sanitize operation. However, as we modified 
> > the code, we found that there was a lot of duplication of
> > code, so we would like to modify the existing background 
> > command handling a little bit. We want to make the most of
> > the existing background command handling so that we can
> > easily add other background commands in the future.
> > 
> > The first patch modifies the existing sanitize operation
> > handling to allow other background commands to be used 
> > with it, and the second patch modifies sanitize to work
> > with the modified code. Finally, the third patch adds a
> > sysfs file related to the background operation. 
> > This sysfs entry is select/poll capable from userspace 
> > to notify upon completion of a background operation.
> > 
> > Thanks,
> > /jtpark
> 
> Hi Jeongtae,
> For the next version of the series you send out, can you please make sure the series is threaded? Currently all your series are coming as individual mails. Thanks!


Sorry about that, I think there a problem of mail security system
at that time. I'll test internally before sending the patch series.

Thanks!

> 
> 
> > 
> > Jeongtae Park (3):
> >   cxl: Modify background cmd handling for the others
> >   cxl/mbox: Update sanitize opcode support
> >   cxl/memdev: Add background command sysfs file
> > 
> >  Documentation/ABI/testing/sysfs-bus-cxl | 10 ++++
> >  drivers/cxl/core/mbox.c                 |  5 +-
> >  drivers/cxl/core/memdev.c               | 71 +++++++++++++++++++++++--
> >  drivers/cxl/cxlmem.h                    | 33 ++++++++++--
> >  drivers/cxl/pci.c                       | 65 +++++++++++-----------
> >  5 files changed, 141 insertions(+), 43 deletions(-)
> > 
> > 
> > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 0/3] cxl: Fix background operation handling

[Jeongtae Park]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

On Wed, Sep 27, 2023 at 06:02:03PM -0700, Davidlohr Bueso wrote:
> On Fri, 22 Sep 2023, Jeongtae Park wrote:
> 
> > Hello,
> > 
> > We are currently trying to add background commands like
> > fw-related and sanitize operation. However, as we modified
> > the code, we found that there was a lot of duplication of
> > code, so we would like to modify the existing background
> > command handling a little bit. We want to make the most of
> > the existing background command handling so that we can
> > easily add other background commands in the future.
> 
> What code duplication? All you need to do is arm the
> struct cxl_mbox_cmd with sensible timeouts. For example,
> see 9521875bbe0 (cxl: add a firmware update mechanism using
> the sysfs firmware loader).
> 
> > The first patch modifies the existing sanitize operation
> > handling to allow other background commands to be used
> > with it, and the second patch modifies sanitize to work
> > with the modified code. Finally, the third patch adds a
> > sysfs file related to the background operation.
> > This sysfs entry is select/poll capable from userspace
> > to notify upon completion of a background operation.
> 
> This says nothing about the motivation of these changes.
> Specially when it has been well discussed in the past that
> exactly because of its monopolizing nature, the driver will
> _only_ do sanitize asynchronously. See the changelog of
> ccadf1310fb (cxl/mbox: Add background cmd handling machinery).
> 
> Thanks,
> Davidlohr

Hello Davidlohr,

I'm afraid that as you mentioned, the cover letter didn't seem to be
written including the problems need to be improved (we are guessing)
and our intentions. I'll review history about them, and make more
specific examples to explain our intentions and how we're going to fix it.

(Sorry for late reply, We've been on long holiday for a while.)

Thanks!
/jtpark

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]