| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <48C8D0C7.6060706@marian.de>
Date: Thu, 11 Sep 2008 10:03:19 +0200
From: Franco Fichtner <franco@...ian.de>
To: Ron Mercer <ron.mercer@...gic.com>
CC: jeff@...zik.org, netdev@...r.kernel.org, linux-driver@...gic.com
Subject: Re: [PATCH 6/8] [RFC] qlge: Add mgmt port xface file qlge_mpi.c
Ron Mercer wrote:
> Signed-off-by: Ron Mercer <ron.mercer@...gic.com>
> ---
> drivers/net/qlge/qlge_mpi.c | 228 +++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 228 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/qlge/qlge_mpi.c
>
> diff --git a/drivers/net/qlge/qlge_mpi.c b/drivers/net/qlge/qlge_mpi.c
> new file mode 100644
> index 0000000..f235fc5
> --- /dev/null
> +++ b/drivers/net/qlge/qlge_mpi.c
> @@ -0,0 +1,228 @@
> +#include "qlge.h"
> +
> +/*
> + * Returns zero on success.
> + */
> +static int ql_wait_mbx_cmd_cmplt(struct ql_adapter *qdev)
> +{
> + int count = 50;
> + u32 temp;
> +
> + while (count) {
> + temp = ql_read32(qdev, STS);
> + if (temp & STS_PI)
> + return 0;
> + mdelay(5);
Do you really want to delay execution for 250ms in worst case (timeout)
scenarios? A msleep() would be more graceful.
> + count--;
> + }
> + return -ETIMEDOUT;
> +}
> +
> +static int ql_write_mbox_reg(struct ql_adapter *qdev, u32 reg, u32 data)
> +{
> + int status = 0;
> + /* wait for reg to come ready */
> + status = ql_wait_addr_reg(qdev, PROC_ADDR, PROC_ADDR_RDY);
> + if (status)
> + goto exit;
> + /* write the data to the data reg */
> + ql_write32(qdev, PROC_DATA, data);
> + /* trigger the write */
> + ql_write32(qdev, PROC_ADDR, reg);
> + /* wait for reg to come ready */
> + status = ql_wait_addr_reg(qdev, PROC_ADDR, PROC_ADDR_RDY);
> + if (status)
> + goto exit;
These two lines above are redundant. You cold also avoid the goto with
opening a new block after the first if. Or how about just...
static int ql_write_mbox_reg(struct ql_adapter *qdev, u32 reg, u32 data)
{
/* wait for reg to come ready */
if (ql_wait_addr_reg(qdev_ PROC_ADDR, PROC_ADDR_RDY)) {
/* write the data to the data reg */
ql_write32(qdev, PROC_DATA, data);
/* trigger the write */
ql_write32(qdev, PROC_ADDR, reg);
}
/* wait for reg to come ready */
return ql_wait_addr_reg(qdev, PROC_ADDR, PROC_ADDR_RDY);
}
... omitting the need for the status variable altogether.
> +exit:
> + return status;
> +}
> +
> +static int ql_read_mbox_reg(struct ql_adapter *qdev, u32 reg, u32 *data)
> +{
> + int status = 0;
Same here for the goto. And no need to initialize status with a constant
which must be pulled from memory when there is no path using this
default value anyway.
> + /* wait for reg to come ready */
> + status = ql_wait_addr_reg(qdev, PROC_ADDR, PROC_ADDR_RDY);
> + if (status)
> + goto exit;
> + /* set up for reg read */
> + ql_write32(qdev, PROC_ADDR, reg | PROC_ADDR_R);
> + /* wait for reg to come ready */
> + status = ql_wait_addr_reg(qdev, PROC_ADDR, PROC_ADDR_RDY);
> + if (status)
> + goto exit;
> + /* get the data */
> + *data = ql_read32(qdev, PROC_DATA);
> +exit:
> + return status;
> +}
> +
> +static int ql_exec_mb_cmd(struct ql_adapter *qdev, struct mbox_params *mbcp)
> +{
> + int i, status;
> +
> + /*
> + * Make sure there's nothing pending.
> + * This shouldn't happen.
> + */
> + if (ql_read32(qdev, CSR) & CSR_HRI)
> + return -EBUSY;
> +
> + status = ql_sem_spinlock(qdev, SEM_PROC_REG_MASK);
> + if (status)
> + return -EBUSY;
> + /*
> + * Fill the outbound mailboxes.
> + */
> + for (i = 0; i < mbcp->in_count; i++) {
> + status =
> + ql_write_mbox_reg(qdev, qdev->mailbox_in + i,
> + mbcp->mbox_in[i]);
> + if (status)
> + goto end;
> + }
> + /*
> + * Wake up the MPI firmware.
> + */
> + ql_write32(qdev, CSR, CSR_CMD_SET_H2R_INT);
> +end:
> + ql_sem_unlock(qdev, SEM_PROC_REG_MASK); /* does flush too */
> + return status;
> +}
> +
> +int ql_get_mb_sts(struct ql_adapter *qdev, struct mbox_params *mbcp)
> +{
> + int i, status = 0;
Again, default value never used.
> +
> + status = ql_sem_spinlock(qdev, SEM_PROC_REG_MASK);
> + if (status)
> + return -EBUSY;
> + for (i = 0; i < mbcp->out_count; i++) {
> + status =
> + ql_read_mbox_reg(qdev, qdev->mailbox_out + i,
> + &mbcp->mbox_out[i]);
> + if (status) {
> + QPRINTK(qdev, DRV, ERR, "Failed mailbox read.\n");
> + break;
> + }
> + }
> + ql_sem_unlock(qdev, SEM_PROC_REG_MASK); /* does flush too */
> + return status;
> +}
> +
> +static void ql_link_up(struct ql_adapter *qdev)
> +{
> + struct mbox_params mbc;
> + struct mbox_params *mbcp = &mbc;
> + mbcp->out_count = 2;
> +
> + if (ql_get_mb_sts(qdev, mbcp))
> + goto exit;
> +
> + qdev->link_status = mbcp->mbox_out[1];
> + QPRINTK(qdev, DRV, ERR, "Link Up.\n");
> + QPRINTK(qdev, DRV, ERR, "Link Status = 0x%.08x.\n", mbcp->mbox_out[1]);
> + if (!netif_carrier_ok(qdev->ndev)) {
> + QPRINTK(qdev, LINK, INFO, "Link is Up.\n");
> + netif_carrier_on(qdev->ndev);
> + netif_wake_queue(qdev->ndev);
> + }
> +exit:
> + /* Clear the MPI firmware status. */
> + ql_write32(qdev, CSR, CSR_CMD_CLR_R2PCI_INT);
> +}
> +
> +static void ql_link_down(struct ql_adapter *qdev)
> +{
> + struct mbox_params mbc;
> + struct mbox_params *mbcp = &mbc;
> + mbcp->out_count = 3;
> +
> + if (ql_get_mb_sts(qdev, mbcp)) {
> + QPRINTK(qdev, DRV, ERR, "Firmware did not initialize!\n");
> + goto exit;
> + }
> +
> + if (netif_carrier_ok(qdev->ndev)) {
> + QPRINTK(qdev, LINK, INFO, "Link is Down.\n");
> + netif_carrier_off(qdev->ndev);
> + netif_stop_queue(qdev->ndev);
> + }
> + QPRINTK(qdev, DRV, ERR, "Link Down.\n");
> + QPRINTK(qdev, DRV, ERR, "Link Status = 0x%.08x.\n", mbcp->mbox_out[1]);
> +exit:
> + /* Clear the MPI firmware status. */
> + ql_write32(qdev, CSR, CSR_CMD_CLR_R2PCI_INT);
> +}
> +
> +static void ql_init_fw_done(struct ql_adapter *qdev)
> +{
> + struct mbox_params mbc;
> + struct mbox_params *mbcp = &mbc;
> + mbcp->out_count = 2;
> +
> + if (ql_get_mb_sts(qdev, mbcp)) {
> + QPRINTK(qdev, DRV, ERR, "Firmware did not initialize!\n");
> + goto exit;
> + }
> + QPRINTK(qdev, DRV, ERR, "Firmware initialized!\n");
> + QPRINTK(qdev, DRV, ERR, "Firmware status = 0x%.08x.\n",
> + mbcp->mbox_out[0]);
> + QPRINTK(qdev, DRV, ERR, "Firmware Revision = 0x%.08x.\n",
> + mbcp->mbox_out[1]);
> +exit:
> + /* Clear the MPI firmware status. */
> + ql_write32(qdev, CSR, CSR_CMD_CLR_R2PCI_INT);
> +}
> +
> +void ql_mpi_work(struct work_struct *work)
> +{
> + struct ql_adapter *qdev =
> + container_of(work, struct ql_adapter, mpi_work.work);
> + struct mbox_params mbc;
> + struct mbox_params *mbcp = &mbc;
> + mbcp->out_count = 1;
> +
> + while (ql_read32(qdev, STS) & STS_PI) {
> + if (ql_get_mb_sts(qdev, mbcp)) {
> + QPRINTK(qdev, DRV, ERR,
> + "Could not read MPI, resetting ASIC!\n");
> + ql_queue_asic_error(qdev);
> + }
> +
> + switch (mbcp->mbox_out[0]) {
> + case AEN_LINK_UP:
> + ql_link_up(qdev);
> + break;
> + case AEN_LINK_DOWN:
> + ql_link_down(qdev);
> + break;
> + case AEN_FW_INIT_DONE:
> +#ifdef PALLADIUM
> +/* Palladium Workaround Start */
> + case 0:
> +/* Palladium Workaround End */
> +#endif
> + ql_init_fw_done(qdev);
> + break;
> + case AEN_FW_INIT_FAIL:
> + break;
No breaking here...
> + case AEN_SYS_ERR:
> + break;
...and here necessary to make things more consistent with the lines
below. (Having no default seems ok here.)
> + case MB_CMD_STS_GOOD:
> + case MB_CMD_STS_INTRMDT:
> + case MB_CMD_STS_ERR:
> + case AEN_IDC_CMPLT:
> + case AEN_IDC_REQ:
> + break;
> +
> + }
> + }
> + ql_enable_completion_interrupt(qdev, 0);
> +}
> +
> +void ql_mpi_reset_work(struct work_struct *work)
> +{
> + struct ql_adapter *qdev =
> + container_of(work, struct ql_adapter, mpi_reset_work.work);
> + printk(KERN_ERR "%s: Enter, qdev = %p..\n", __func__, qdev);
> +}
Another inconsistency: you're not using QPRINTK() like before. Is this
intended?
Franco
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists