From: "Ahmed S. Darwish" <darwish.07@gmail.com>
To: "Thomas Körper" <thomas.koerper@esd.eu>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org
Subject: Re: [PATCH V6 1/1] can: Add support for esd CAN PCIe/402 card
Date: Tue, 17 Mar 2015 23:55:58 +0200 [thread overview]
Message-ID: <20150317215558.GA2378@Darwish.PC> (raw)
In-Reply-To: <1426592308-23817-1-git-send-email-thomas.koerper@esd.eu>
On Tue, Mar 17, 2015 at 12:38:28PM +0100, Thomas Körper wrote:
...
> +static int pci402_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + struct pci402_card *card = NULL;
> + int err;
> +
> + BUILD_BUG_ON(PCI402_DMA_FIFO_ITEMSIZE != sizeof(struct acc_bmmsg));
> +
> + err = pci_enable_device(pdev);
> + if (err)
> + goto failure;
> +
> + pci_set_master(pdev);
> +
> + card = kzalloc(sizeof(*card), GFP_KERNEL);
> + if (!card)
> + goto failure;
> +
I think there's a resource leak here: the second "goto failure"
should be a "goto failure_disable_pci" instead. That way, in
case of kzalloc() -ENOMEM we can call pci_disable_device() and
decrement the reference counter increased by pci_enable_device()
above...
If so, this can be fixed by making the recovery path be
more straightforward:
err = pci_enable_device(pdev);
if (err)
return err;
card = kzalloc(..., GFP)
if (!card)
goto failure_disable_pci;
err = pci_request_regions(...);
if (err)
goto failure_free_card; /* new label, above failure_disable_pci */
...
> +failure_release_regions:
> + pci_release_regions(pdev);
> +
> +failure_disable_pci:
> + pci_disable_device(pdev);
> +
> +failure:
> + kfree(card);
> +
> + return err;
> +}
> +
...
> +netdev_tx_t acc_start_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> + struct acc_net_priv *priv = netdev_priv(netdev);
> + struct acc_core *core = priv->core;
> + struct can_frame *cf = (struct can_frame *)skb->data;
> + u8 new_fifo_head = (core->tx_fifo_head + 1) % core->tx_fifo_size;
> + u32 esd_id;
> + u8 esd_len;
> +
> + if ((new_fifo_head == core->tx_fifo_tail) || !acc_txq_isready(core)) {
> + netif_stop_queue(netdev);
> + return NETDEV_TX_BUSY;
> + }
> +
Keeping aside the issue outlined in earlier email where there's
no code snippet in this patch that stops the queue but returns
NETDEV_TX_OK as expected..
Is there anything I'm missing that can stop below scenario from
happening:
start_xmit [softirq context] handle_core_msg_rxtxdone [hardirq]
**************************** **********************************
if (new_fifo.. || !acc_txq..) {
PCI IRQ!
-------->
...
netif_wake_queue(core->net_dev);
return;
<--------
netif_stop_queue(netdev);
return NETDEV_TX_BUSY;
}
> + esd_len = can_dlc2len(cf->can_dlc);
> + if (cf->can_id & CAN_RTR_FLAG)
> + esd_len |= ACC_CAN_RTR_FLAG;
> +
> + if (cf->can_id & CAN_EFF_FLAG) {
> + esd_id = cf->can_id & CAN_EFF_MASK;
> + esd_id |= ACC_CAN_EFF_FLAG;
> + } else {
> + esd_id = cf->can_id & CAN_SFF_MASK;
> + }
> +
> + can_put_echo_skb(skb, netdev, core->tx_fifo_head);
> + core->tx_fifo_head = new_fifo_head;
> +
> + acc_txq_put(core, esd_id, esd_len, cf->data);
> +
> + return NETDEV_TX_OK;
> +}
> +
...
> +static void handle_core_msg_rxtxdone(struct acc_core *core,
> + const struct acc_bmmsg_rxtxdone *msg)
> +{
> + struct acc_net_priv *priv = netdev_priv(core->net_dev);
> + struct net_device_stats *stats = &core->net_dev->stats;
> +
> + if (msg->dlc.rxtx.len & ACC_BM_LENFLAG_TX) {
> + if (core->tx_fifo_head == core->tx_fifo_tail) {
> + netdev_warn(core->net_dev,
> + "TX interrupt, but queue is empty!?\n");
> + return;
> + }
> + stats->tx_packets++;
> + stats->tx_bytes +=
> + get_can_dlc(msg->dlc.tx.len & ACC_CAN_DLC_MASK);
> +
> + can_get_echo_skb(core->net_dev, core->tx_fifo_tail);
> + core->tx_fifo_tail++;
> + if (core->tx_fifo_tail >= core->tx_fifo_size)
> + core->tx_fifo_tail = 0;
> + netif_wake_queue(core->net_dev);
> +
> + } else {
> + struct skb_shared_hwtstamps *skb_ts;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> +
> + skb = alloc_can_skb(core->net_dev, &cf);
> + if (!skb) {
> + stats->rx_dropped++;
> + return;
> + }
> +
> + cf->can_id = msg->id & CAN_EFF_MASK;
> + if (msg->id & ACC_CAN_EFF_FLAG)
> + cf->can_id |= CAN_EFF_FLAG;
> +
> + cf->can_dlc = get_can_dlc(msg->dlc.rx.len);
> + if (msg->dlc.rx.len & ACC_CAN_RTR_FLAG)
> + cf->can_id |= CAN_RTR_FLAG;
> + else
> + memcpy(cf->data, msg->data, cf->can_dlc);
> +
> + skb_ts = skb_hwtstamps(skb);
> + skb_ts->hwtstamp = acc_ts_to_ktime(priv->ov, msg->timestamp);
> + netif_rx(skb);
> +
> + stats->rx_packets++;
> + stats->rx_bytes += cf->can_dlc;
> + }
> +}
> +
...
Thanks,
Darwish
prev parent reply other threads:[~2015-03-17 21:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-17 11:38 [PATCH V6 1/1] can: Add support for esd CAN PCIe/402 card Thomas Körper
2015-03-17 13:33 ` Ahmed S. Darwish
2015-03-17 14:51 ` Ahmed S. Darwish
2015-03-17 21:55 ` Ahmed S. Darwish [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150317215558.GA2378@Darwish.PC \
--to=darwish.07@gmail.com \
--cc=linux-can@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=thomas.koerper@esd.eu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).