Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Mon, 7 Feb 2022 11:56:13 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Li Chen <lchen.firstlove@zohomail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        =?utf-8?Q?=22Krzysztof_Wilczy=C5=84ski=22?= <kw@linux.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Bjorn Helgaas <bhelgaas@google.com>,
        linux-pci <linux-pci@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V6] misc: pci_endpoint_test: simplify endpoint test read
 and write operations
Message-ID: <20220207175613.GA404082@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <17ed3719e68.113a2df6350634.5087699168706664746@zohomail.com>
Precedence: bulk

On Mon, Feb 07, 2022 at 04:09:05AM -0500, Li Chen wrote:
> From: Li Chen <lchen@ambarella.com>
> 
> Introduce pci_endpoint_epf_transfer_data to simplify
> read and write operations.
> 
> Also tabify this file.

Thanks for the patch.

This doesn't apply cleanly on v5.17-rc1.  Please make it apply cleanly
there or at least mention where it *does* apply.

Please separate the whitespace tabification changes and the
pci_endpoint_epf_transfer_data() changes into two separate patches.
When they're mixed together, it's harder to review the patch.

> #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
> -                     miscdev)
> +                        miscdev)

Always indent with tabs when possible:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n18

Hmm, coding-style.rst is unfortunately not very explicit about that.

But it's obvious from the existing code in this file that things
should not be indented four spaces, as you did in
pci_endpoint_test_transfer_data().

Your patch should match the style of the existing code.

> +static bool pci_endpoint_test_transfer_data(struct pci_endpoint_test *test,
> +                unsigned long arg, const int operation)
> +{
> +    struct pci_endpoint_test_xfer_param param;
> +    bool ret = false;
> +    u32 flags = 0;


> +    // if we ask rc to write to ep, then ep should do read operation, and vice versa.

Please use /* */ comments to match the prevailing kernel comment
style:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=v5.16#n598

And spell out or at least capitalize "RC" and "EP" since they're not
real words.

Bjorn