On Thu, Sep 03, 2020 at 12:49:18PM +0900, Benjamin Poirier wrote:
On 2020-09-02 22:00 +0800, Coiby Xu wrote:This fixes commit 0107635e15ac ("staging: qlge: replace pr_err with netdev_err") which introduced an build breakage of missing `struct ql_adapter *qdev` for some functions and a warning of type mismatch with dumping enabled, i.e., $ make CFLAGS_MODULE="QL_ALL_DUMP=1 QL_OB_DUMP=1 QL_CB_DUMP=1 \ QL_IB_DUMP=1 QL_REG_DUMP=1 QL_DEV_DUMP=1" M=drivers/staging/qlge qlge_dbg.c: In function ‘ql_dump_ob_mac_rsp’: qlge_dbg.c:2051:13: error: ‘qdev’ undeclared (first use in this function); did you mean ‘cdev’? 2051 | netdev_err(qdev->ndev, "%s\n", __func__); | ^~~~ qlge_dbg.c: In function ‘ql_dump_routing_entries’: qlge_dbg.c:1435:10: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘int’ [-Wformat=] 1435 | "%s: Routing Mask %d = 0x%.08x\n", | ~^ | | | char * | %d 1436 | i, value); | ~ | | | int qlge_dbg.c:1435:37: warning: format ‘%x’ expects a matching ‘unsigned int’ argument [-Wformat=] 1435 | "%s: Routing Mask %d = 0x%.08x\n", | ~~~~^ | | | unsigned int Fixes: 0107635e15ac ("staging: qlge: replace pr_err with netdev_err") Reported-by: Benjamin Poirier <benjamin.poirier@xxxxxxxxx> Suggested-by: Benjamin Poirier <benjamin.poirier@xxxxxxxxx> Signed-off-by: Coiby Xu <coiby.xu@xxxxxxxxx> ---Thanks for following up on this issue. [...]@@ -1632,8 +1635,8 @@ void ql_dump_wqicb(struct wqicb *wqicb) void ql_dump_tx_ring(struct tx_ring *tx_ring) { - if (!tx_ring) - return; + struct ql_adapter *qdev = tx_ring->qdev; + netdev_err(qdev->ndev, "===================== Dumping tx_ring %d ===============\n", tx_ring->wq_id); netdev_err(qdev->ndev, "tx_ring->base = %p\n", tx_ring->wq_base);Did you actually check to confirm that the test can be removed?
Thank you for the reminding! For the current code, when ql_dump_tx_ring is called, tx_ring would never be null.
This is something that you should mention in the changelog at the very least since that change is not directly about fixing the build breakage and if it's wrong, it can lead to null pointer deref.
I thought it is common practice in C that the caller makes sure the passed parameter isn't a null pointer because a QEMU developer also gave me the same advice after reviewing one of my patches for QEMU a few weeks ago. I'll mention this in the commit message. Thank you for the suggestion! -- Best regards, Coiby _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
