On Mon, Mar 03, 2025 at 07:21:28AM +0100, Michal Swiatkowski wrote:
> > @@ -523,7 +524,7 @@ qed_ll2_rxq_handle_completion(struct qed_hwfn *p_hwfn,
> > static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
> > {
> > struct qed_ll2_info *p_ll2_conn = (struct qed_ll2_info *)cookie;
> > - struct qed_ll2_rx_queue *p_rx = &p_ll2_conn->rx_queue;
> > + struct qed_ll2_rx_queue *p_rx;
> > union core_rx_cqe_union *cqe = NULL;
> > u16 cq_new_idx = 0, cq_old_idx = 0;
> > unsigned long flags = 0;
> > @@ -532,6 +533,7 @@ static int qed_ll2_rxq_completion(struct qed_hwfn *p_hwfn, void *cookie)
> > if (!p_ll2_conn)
> > return rc;
> >
> > + p_rx = &p_ll2_conn->rx_queue;
> > spin_lock_irqsave(&p_rx->lock, flags);
> >
> > if (!QED_LL2_RX_REGISTERED(p_ll2_conn)) {
>
> For future submission plase specify the target kernel
> [PATCH net] for fixes, [PATCH net-next] for other.
>
> Looking at the code callback is always set together with cookie (which
> is pointing to p_ll2_conn. I am not sure if this is fixing real issue,
> but maybe there are a cases when callback is still connected and cookie
> is NULL.
The assignment:
p_rx = &p_ll2_conn->rx_queue;
does not dereference "p_ll2_conn". It just does pointer math. So the
original code works fine.
The question is, did the original author write it that way intentionally?
I've had this conversation with people before and they eventually are
convinced that "Okay, the code works, but only by sheer chance." In my
experiences, most of the time, the authors of this type of code are so
used to weirdnesses of C that they write code like this without even
thinking about it. It never even occurs to them how it could be
confusing.
This commit message is misleading and there should not be a Fixes tag.
regards,
dan carpenter
