Re: [PATCH 08/10] esp: convert ESPState pdma_cb from a function pointer to an integer

[Mark Cave-Ayland]

On 28/02/2022 23:10, Philippe Mathieu-Daudé wrote:

> On 28/2/22 23:25, Mark Cave-Ayland wrote:
> > This prepares for the inclusion of the current PDMA callback in the migration
> > stream since the callback is referenced by an integer instead of a function
> > pointer.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hw/scsi/esp.c         | 47 ++++++++++++++++++++++++++++++-------------
> >   include/hw/scsi/esp.h | 11 +++++++++-
> >   2 files changed, 43 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> > index 035fb0d1f6..e8b6f25dbb 100644
> > --- a/hw/scsi/esp.c
> > +++ b/hw/scsi/esp.c
> > @@ -195,14 +195,10 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
> >       esp_set_tc(s, dmalen);
> >   }
> > -static void esp_set_pdma_cb(ESPState *s, void (*cb)(ESPState *))
> > +static void esp_set_pdma_cb(ESPState *s, int pdma_cb)
>
> Why signed?

Ooops.

> >   {
> > -    s->pdma_cb = cb;
> > -}
> > -
> > -static void esp_pdma_cb(ESPState *s)
> > -{
> > -    s->pdma_cb(s);
> > +    assert(pdma_cb >= 0 && pdma_cb < PDMA_NUM_CBS);
>
> No need to check >=0 if using unsigned.

Agreed.

> > +    s->pdma_cb = pdma_cb;
> >   }
>
> > +static void esp_pdma_cb(ESPState *s)
> > +{
> > +    switch (s->pdma_cb) {
> > +    case PDMA_SATN_PDMA_CB:
> > +        satn_pdma_cb(s);
> > +        break;
> > +    case PDMA_S_WITHOUT_SATN_PDMA_CB:
> > +        s_without_satn_pdma_cb(s);
> > +        break;
> > +    case PDMA_SATN_STOP_PDMA_CB:
> > +        satn_stop_pdma_cb(s);
> > +        break;
> > +    case PDMA_WRITE_RESPONSE_PDMA_CB:
> > +        write_response_pdma_cb(s);
> > +        break;
> > +    case PDMA_DO_DMA_PDMA_CB:
> > +        do_dma_pdma_cb(s);
> > +        break;
> > +    default:
> > +        g_assert_not_reached();
> > +    }
> > +}
> > +
>
> > index b1ec27612f..885ccf4660 100644
> > --- a/include/hw/scsi/esp.h
> > +++ b/include/hw/scsi/esp.h
> > @@ -51,7 +51,7 @@ struct ESPState {
> >       ESPDMAMemoryReadWriteFunc dma_memory_write;
> >       void *dma_opaque;
> >       void (*dma_cb)(ESPState *s);
> > -    void (*pdma_cb)(ESPState *s);
> > +    uint8_t pdma_cb;
>
> And here you use unsigned :)
>
> >       uint8_t mig_version_id;
> > @@ -150,6 +150,15 @@ struct SysBusESPState {
> >   #define TCHI_FAS100A 0x4
> >   #define TCHI_AM53C974 0x12
> > +/* PDMA callbacks */
> > +#define PDMA_SATN_PDMA_CB              0
> > +#define PDMA_S_WITHOUT_SATN_PDMA_CB    1
> > +#define PDMA_SATN_STOP_PDMA_CB         2
> > +#define PDMA_WRITE_RESPONSE_PDMA_CB    3
> > +#define PDMA_DO_DMA_PDMA_CB            4
>
> I'd rather use an enum (and in esp_pdma_cb's switch).
>
> > +#define PDMA_NUM_CBS                   5
> > +
> >   void esp_dma_enable(ESPState *s, int irq, int level);
> >   void esp_request_cancelled(SCSIRequest *req);
> >   void esp_command_complete(SCSIRequest *req, size_t resid);
>
> Preferrably using unsigned:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I'll aim to keep the unsigned type in ESPState, and experiment with changing the 
#defines to an enum. If it looks good, I'll include the enum version in v2.


ATB,

Mark.