Re: [PATCH] stm: class: Add MIPI OST protocol support

[Jinlong Mao]

On 3/7/2023 9:26 PM, Jinlong Mao wrote:

> Hi Alexander,
Sorry, correct the typo.
> On 3/3/2023 2:05 AM, Alexander Shishkin wrote:
> > Mao Jinlong <quic_jinlmao@xxxxxxxxxxx> writes:
> >
> > > Add MIPI OST protocol support for stm to format the traces.
> > Missing an explanation of what OST is, what it's used for, how it is
> > different from the SyS-T and others.
> I will updae the explanation in next version.
> > > Framework copied from drivers/hwtracing/stm.p-sys-t.c as of
> > You mean stm/p_sys-t.c. Also, it's not a framework, it's a driver.
>
> The driver refers to code structure of p_sys-t driver. So, add this 
> comments after
> internal review.
>
> > > commit d69d5e83110f ("stm class: Add MIPI SyS-T protocol
> > > support").
> > Why is this significant?
> >
> > > diff --git a/drivers/hwtracing/stm/p_ost.c 
> > > b/drivers/hwtracing/stm/p_ost.c
> > > new file mode 100644
> > > index 000000000000..2ca1a3fda57f
> > > --- /dev/null
> > > +++ b/drivers/hwtracing/stm/p_ost.c
> > > @@ -0,0 +1,95 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copied from drivers/hwtracing/stm.p-sys-t.c as of commit 
> > > d69d5e83110f
> > > + * ("stm class: Add MIPI SyS-T protocol support").
> > Same as in the commit message.
> >
> > [...]
> >
> > > +#define OST_TOKEN_STARTSIMPLE (0x10)
> > > +#define OST_VERSION_MIPI1        (0x10 << 8)
> > > +#define OST_ENTITY_FTRACE        (0x01 << 16)
> > > +#define OST_CONTROL_PROTOCOL        (0x0 << 24)
> > These could use an explanation.
> I will add the explanation.
> > > +#define DATA_HEADER (OST_TOKEN_STARTSIMPLE | OST_VERSION_MIPI1 | \
> > > +            OST_ENTITY_FTRACE | OST_CONTROL_PROTOCOL)
> > Does this mean that everything is ftrace? Because it's not.
> Only ftrace is supported in p_ost now. Other header type will be added 
> later.
> > > +
> > > +#define STM_MAKE_VERSION(ma, mi)    ((ma << 8) | mi)
> > > +#define STM_HEADER_MAGIC        (0x5953)
> > > +
> > > +static ssize_t notrace ost_write(struct stm_data *data,
> > > +        struct stm_output *output, unsigned int chan,
> > > +        const char *buf, size_t count)
> > > +{
> > > +    unsigned int c = output->channel + chan;
> > > +    unsigned int m = output->master;
> > > +    const unsigned char nil = 0;
> > > +    u32 header = DATA_HEADER;
> > > +    u8 trc_hdr[24];
> > > +    ssize_t sz;
> > > +
> > > +    /*
> > > +     * STP framing rules for OST frames:
> > > +     *   * the first packet of the OST frame is marked;
> > > +     *   * the last packet is a FLAG.
> > Which in your case is also timestamped.
> I will add the comments.
> > > +     */
> > > +    /* Message layout: HEADER / DATA / TAIL */
> > > +    /* HEADER */
> > > +
> > > +    sz = data->packet(data, m, c, STP_PACKET_DATA, STP_PACKET_MARKED,
> > > +              4, (u8 *)&header);
> > The /* HEADER */ comment applies to the above line, so it should
> > probably be directly before it.
> Got it.
> > > +    if (sz <= 0)
> > > +        return sz;
> > > +    *(uint16_t *)(trc_hdr) = STM_MAKE_VERSION(0, 3);
> > > +    *(uint16_t *)(trc_hdr + 2) = STM_HEADER_MAGIC;
> > > +    *(uint32_t *)(trc_hdr + 4) = raw_smp_processor_id();
> > > +    *(uint64_t *)(trc_hdr + 8) = sched_clock();
> > Why sched_clock()? It should, among other things, be called with
> > interrupts disabled, which is not the case here.
> I will check. If it is not necessary here, I will remove it.
> > > +    *(uint64_t *)(trc_hdr + 16) = task_tgid_nr(get_current());
> > Is there a reason why trc_hdr is not a struct?
> No particular reason here.
> > Thanks,
> > --
> > Alex