lkml.org 
[lkml]   [2021]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 2/3] staging: r8188eu: simplify c2h_evt_hdl function
    On Sun, 29 Aug 2021 at 09:52, Fabio M. De Francesco
    <fmdefrancesco@gmail.com> wrote:
    >
    > > static s32 c2h_evt_hdl(struct adapter *adapter, struct c2h_evt_hdr *c2h_evt, c2h_id_filter filter)
    > > {
    > > - s32 ret = _FAIL;
    > > u8 buf[16];
    > >
    > > - if (!c2h_evt) {
    > > - /* No c2h event in cmd_obj, read c2h event before handling*/
    > > - if (c2h_evt_read(adapter, buf) == _SUCCESS) {
    > > - c2h_evt = (struct c2h_evt_hdr *)buf;
    >
    > Dear Philip,
    >
    > Not related to your patch, but what kind of odd assignment is it? c2h_evt takes
    > the address of a local variable and therefore it crashes the kernel whenever
    > someone decides to dereference it after this function returns and unwinds
    > the stack...

    Dear Fabio,

    Thank you for taking a look firstly, really appreciate it :-) As for the line:
    c2h_evt = (struct c2h_evt_hdr *)buf;

    in the original code before I removed it, bear in mind that this
    pointer assignment is
    happening into the parameter variable c2h_evt, which is a copy of the
    passed in argument,
    as C is pass-by-value. Therefore, after the c2h_evt_hdl function
    returns, the value passed
    in as the argument for this parameter would still have its original
    pointer value (or NULL).

    It would not therefore be possible to deference the pointer to this
    stack-allocated memory
    from outside the function, even in the original code. I agree though,
    its purpose is dubious.
    Originally, the wrapper function rtw_hal_c2h_handler would have passed
    it through to the
    function assigned to the c2h_handler function pointer, but there was
    no such function in
    this driver, so it was never executed.

    >
    > > + if (!c2h_evt)
    > > + c2h_evt_read(adapter, buf);
    >
    > Having said that, I strongly doubt that this path is ever taken. I didn't check the call
    > chain, but it may be that the function in never called or, if it is called, it always
    > has a valid c2h_evt argument.
    >
    > Actually I don't mean to suggest something specific. It simply looks odd, so I'd check
    > and if this happens to be the case, I'd remove the whole c2h_evt_hdl().
    >
    > Regards,
    >
    > Fabio

    As alluded to, removing the whole of c2h_evt_hdl would lead to
    c2h_evt_read no longer
    being executed, which would mean the reads from the adapter register
    don't happen, and
    nor does the clearing by c2h_evt_clear(adapter); - in particular, the
    comment there mentions
    the FW not updating the next command message if this isn't executed when needed.

    This may be perfectly fine, but I thought this approach is safer due
    to the above.

    Regards,
    Phil

    \
     
     \ /
      Last update: 2021-08-29 12:50    [W:2.575 / U:0.200 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site