| 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: <20081210230314.GB30537@electric-eye.fr.zoreil.com>
Date: Thu, 11 Dec 2008 00:03:14 +0100
From: Francois Romieu <romieu@...zoreil.com>
To: Sathya Perla <sathyap@...verengines.com>
Cc: netdev <netdev@...r.kernel.org>, jgarzik@...ox.com,
jeff@...zik.org, subbu <subbus@...verengines.com>
Subject: Re: [PATCH 01/11] benet: header and init functions
Sathya Perla <sathyap@...verengines.com> :
[...]
> diff --git a/drivers/net/benet/be_init.c b/drivers/net/benet/be_init.c
> new file mode 100644
> index 0000000..06fb343
> --- /dev/null
> +++ b/drivers/net/benet/be_init.c
[...]
> +static int
> +init_pci_be_function(struct be_adapter *adapter, struct pci_dev *pdev)
> +{
> + u64 pa;
> +
> + /* CSR */
> + pa = pci_resource_start(pdev, 2);
> + adapter->csr_va = ioremap_nocache(pa, pci_resource_len(pdev, 2));
> + if (adapter->csr_va == NULL)
> + return -ENOMEM;
> +
> + /* Door Bell */
> + pa = pci_resource_start(pdev, 4);
> + adapter->db_va = ioremap_nocache(pa, (128 * 1024));
> + if (adapter->db_va == NULL) {
> + iounmap(adapter->csr_va);
> + return -ENOMEM;
> + }
> +
> + /* PCI */
> + pa = pci_resource_start(pdev, 1);
> + adapter->pci_va = ioremap_nocache(pa, pci_resource_len(pdev, 1));
> + if (adapter->pci_va == NULL) {
> + iounmap(adapter->csr_va);
> + iounmap(adapter->db_va);
> + return -ENOMEM;
> + }
This error path could use gotos.
[...]
> +static int be_register_isr(struct be_adapter *adapter,
> + struct be_net_object *pnob)
> +{
> + struct net_device *netdev = pnob->netdev;
> + int intx = 0, r;
> +
> + netdev->irq = adapter->pdev->irq;
> + r = be_enable_msix(adapter);
> +
> + if (r == 0) {
> + r = request_irq(adapter->msix_entries[0].vector,
> + be_int, IRQF_SHARED, netdev->name, netdev);
> + if (r) {
> + printk(KERN_WARNING
> + "MSIX Request IRQ failed - Errno %d\n", r);
This printk will be of moderate help in the middle of a kilometer long
dmesg. You may use dev_warn and friends to identify the emitter of the
warning.
> + intx = 1;
> + pci_disable_msix(adapter->pdev);
> + adapter->msix_enabled = 0;
> + }
> + } else {
> + intx = 1;
> + }
> +
> + if (intx) {
> + r = request_irq(netdev->irq, be_int, IRQF_SHARED,
> + netdev->name, netdev);
be_int is not known in this patch.
> + if (r) {
> + printk(KERN_WARNING
> + "INTx Request IRQ failed - Errno %d\n", r);
> + return -1;
Please propagate "r" as a status code up to the highest level caller.
If you really need some error status code of you own, you may
consider using Exxx (ENOMEM, EBUSY, etc.) ones.
[...]
> +static void be_rx_q_clean(struct be_net_object *pnob)
> +{
> + if (pnob->rx_ctxt) {
> + int i;
> + struct be_rx_page_info *rx_page_info;
> + for (i = 0; i < pnob->rx_q_len; i++) {
Please insert a blank line between the declaration and the code.
[...]
> +static int be_nob_ring_alloc(struct be_adapter *adapter,
> + struct be_net_object *pnob)
> +{
> + u32 size;
> +
> + /* Mail box rd; mailbox pointer needs to be 16 byte aligned */
> + pnob->mb_size = sizeof(struct MCC_MAILBOX_AMAP) + 16;
> + pnob->mb_ptr = pci_alloc_consistent(adapter->pdev, pnob->mb_size,
> + &pnob->mb_bus);
> + if (!pnob->mb_bus)
> + return -1;
return -ENOMEM ?
[...]
> +static void be_remove(struct pci_dev *pdev)
> +{
> + struct be_net_object *pnob;
> + struct be_adapter *adapter;
> +
> + adapter = pci_get_drvdata(pdev);
> + if (!adapter)
> + return;
> +
> + pci_set_drvdata(pdev, NULL);
> + pnob = (struct be_net_object *)adapter->net_obj;
> +
> + flush_scheduled_work();
> +
> + if (pnob) {
> + /* Unregister async callback function for link status updates */
> + if (pnob->mcc_q_created)
> + be_mcc_add_async_event_callback(&pnob->mcc_q_obj,
be_mcc_add_async_event_callback is not defined in this patch.
[...]
> +static int be_probe(struct pci_dev *pdev, const struct pci_device_id *pdev_id)
> +{
> + int status = 0;
Useless initialization.
> + struct be_adapter *adapter;
> + struct FWCMD_COMMON_GET_FW_VERSION_RESPONSE_PAYLOAD get_fwv;
> + struct be_net_object *pnob;
> + struct net_device *netdev;
> +
> + status = pci_enable_device(pdev);
> + if (status)
> + goto error;
> +
> + status = pci_request_regions(pdev, be_driver_name);
> + if (status)
> + goto error_pci_req;
> +
> + pci_set_master(pdev);
The adapter is never used with bioses which could leave it ready
to DMA into memory before linux is started, right ?
Otherwise we are in trouble.
[...]
> + status = be_register_isr(adapter, pnob);
> + if (status != 0)
> + goto cleanup;
I have not seen where the device is reset before registering the irq.
[...]
> +void be_update_link_status(struct be_adapter *adapter)
> +{
> + int status;
> + struct be_net_object *pnob = adapter->net_obj;
> +
> + status = be_rxf_link_status(&pnob->fn_obj, adapter->be_link_sts, NULL,
be_rxf_link_status is not defined in this patch.
[...]
> +static int be_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + struct be_adapter *adapter = pci_get_drvdata(pdev);
> + struct net_device *netdev = adapter->netdevp;
> + struct be_net_object *pnob = (struct be_net_object *)netdev->priv;
> +
> + adapter->dev_pm_state = adapter->dev_state;
> + adapter->dev_state = BE_DEV_STATE_SUSPEND;
> +
> + netif_device_detach(netdev);
> + if (netif_running(netdev))
> + be_pm_cleanup(adapter, pnob, netdev);
> +
> + pci_enable_wake(pdev, 3, 1);
> + pci_enable_wake(pdev, 4, 1); /* D3 Cold = 4 */
You can use PCI_D3hot and PCI_D3cold.
--
Ueimor
--
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