linux-kernel - Re: [PATCH v3 0/8] Hey Jassi, all,

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d3fe02e9-0e34-17d0-251e-5cc4394c8889@microchip.com>
Date:   Mon, 3 Apr 2023 18:11:32 +0000
From:   <Valentina.FernandezAlanis@...rochip.com>
To:     <conor@...nel.org>, <linux-riscv@...ts.infradead.org>
CC:     <Conor.Dooley@...rochip.com>, <Daire.McNamara@...rochip.com>,
        <jassisinghbrar@...il.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 0/8] Hey Jassi, all,

On 07/03/2023 20:22, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@...rochip.com>
> 
> Here are some fixes for the system controller on PolarFire SoC that I
> ran into while implementing support for using the system controller to
> re-program the FPGA. A few are just minor bits that I fixed in passing,
> but the bulk of the patchset is changes to how the mailbox figures out
> if a "service" has completed.
> 
> Prior to implementing this particular functionality, the services
> requested from the system controller, via its mailbox interface, always
> triggered an interrupt when the system controller was finished with
> the service.
> 
> Unfortunately some of the services used to validate the FPGA images
> before programming them do not trigger an interrupt if they fail.
> For example, the service that checks whether an FPGA image is actually
> a newer version than what is already programmed, does not trigger an
> interrupt, unless the image is actually newer than the one currently
> programmed. If it has an earlier version, no interrupt is triggered
> and a status is set in the system controller's status register to
> signify the reason for the failure.
> 
> In order to differentiate between the service succeeding & the system
> controller being inoperative or otherwise unable to function, I had to
> switch the controller to poll a busy bit in the system controller's
> registers to see if it has completed a service.
> This makes sense anyway, as the interrupt corresponds to "data ready"
> rather than "tx done", so I have changed the mailbox controller driver
> to do that & left the interrupt solely for signalling data ready.
> It just so happened that all of the services that I had worked with and
> tested up to this point were "infallible" & did not set a status, so the
> particular code paths were never tested.
> 
> Jassi, the mailbox and soc patches depend on each other, as the change
> in what the interrupt is used for requires changing the client driver's
> behaviour too, as mbox_send_message() will now return when the system
> controller is no longer busy rather than when the data is ready.
> I'm happy to send the lot via the soc tree with your Ack and/or reivew,
> if that also works you?
> I've got some other bits that I'd like to change in the client driver,
> so via the soc tree would suit me better.
> 
> Thanks,
> Conor.
Hi Conor,

I tested this on the Icicle Kit board, looks good to me. So:
Tested-by: Valentina Fernandez <valentina.fernandezalanis@...rochip.com>
> 
> Changes in v3:
> - check the service status in the .tx_done() callback rather than
>    mpfs_mbox_rx_data()
> - re-order the if/else bits in mpfs_blocking_transaction() to please my
>    eyes a bit more
> - expand on the comment in same
> 
> Changes in v2:
> - up the timeout to 30 seconds, as required for services like image
>    validation, which may vary significantly in execution time
> - fixed a typo!
> 
> CC: Conor Dooley <conor.dooley@...rochip.com>
> CC: Daire McNamara <daire.mcnamara@...rochip.com>
> CC: Jassi Brar <jassisinghbrar@...il.com>
> CC: linux-riscv@...ts.infradead.org
> CC: linux-kernel@...r.kernel.org
> 
> Conor Dooley (8):
>    mailbox: mpfs: fix an incorrect mask width
>    mailbox: mpfs: switch to txdone_poll
>    mailbox: mpfs: ditch a useless busy check
>    mailbox: mpfs: check the service status in .tx_done()
>    soc: microchip: mpfs: fix some horrible alignment
>    soc: microchip: mpfs: use a consistent completion timeout
>    soc: microchip: mpfs: simplify error handling in
>      mpfs_blocking_transaction()
>    soc: microchip: mpfs: handle timeouts and failed services differently
> 
>   drivers/mailbox/mailbox-mpfs.c              | 55 ++++++++++++---------
>   drivers/soc/microchip/mpfs-sys-controller.c | 52 +++++++++++++------
>   2 files changed, 67 insertions(+), 40 deletions(-)
>