From: gregkh@linuxfoundation.org (Greg Kroah-Hartman)
Subject: [RFC 1/1] MicroSemi Switchtec management interface driver
Date: Sat, 17 Dec 2016 23:51:41 -0800 [thread overview]
Message-ID: <20161218075141.GE29850@kroah.com> (raw)
In-Reply-To: <1481994562-9283-2-git-send-email-logang@deltatee.com>
On Sat, Dec 17, 2016@10:09:22AM -0700, Logan Gunthorpe wrote:
> +struct switchtec_dev {
> + struct pci_dev *pdev;
> + struct msix_entry *msix;
> + struct device *dev;
> + struct kref kref;
Why do you have a pointer to a device, yet a kref as well? Just have
this structure embed a 'struct device' in itself, like you did for a
kref, and you will be fine. Otherwise you are dealing with two
different sets of reference counting here, for no good reason.
> +
> + unsigned int event_irq;
> +
> + void __iomem *mmio;
> + struct mrpc_regs __iomem *mmio_mrpc;
> + struct ntb_info_regs __iomem *mmio_ntb;
> + struct part_cfg_regs __iomem *mmio_part_cfg;
> +
> + /*
> + * The mrpc mutex must be held when accessing the other
> + * mrpc fields
> + */
> + struct mutex mrpc_mutex;
> + struct list_head mrpc_queue;
> + int mrpc_busy;
> + struct work_struct mrpc_work;
> + struct delayed_work mrpc_timeout;
> +};
> +
> +#define stdev_pdev(stdev) ((stdev)->pdev)
> +#define stdev_pdev_dev(stdev) (&stdev_pdev(stdev)->dev)
> +#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
> +#define stdev_dev(stdev) ((stdev)->dev)
Ick, just open code these please. That's a huge hint your use of the
driver model is not correct :)
thanks,
greg k-h
WARNING: multiple messages have this Message-ID (diff)
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Jonathan Corbet <corbet@lwn.net>,
"David S. Miller" <davem@davemloft.net>,
Andrew Morton <akpm@linux-foundation.org>,
Emil Velikov <emil.l.velikov@gmail.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Guenter Roeck <linux@roeck-us.net>,
Kurt Schwemmer <kurt.schwemmer@microsemi.com>,
Stephen Bates <stephen.bates@microsemi.com>,
linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC 1/1] MicroSemi Switchtec management interface driver
Date: Sat, 17 Dec 2016 23:51:41 -0800 [thread overview]
Message-ID: <20161218075141.GE29850@kroah.com> (raw)
In-Reply-To: <1481994562-9283-2-git-send-email-logang@deltatee.com>
On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> +struct switchtec_dev {
> + struct pci_dev *pdev;
> + struct msix_entry *msix;
> + struct device *dev;
> + struct kref kref;
Why do you have a pointer to a device, yet a kref as well? Just have
this structure embed a 'struct device' in itself, like you did for a
kref, and you will be fine. Otherwise you are dealing with two
different sets of reference counting here, for no good reason.
> +
> + unsigned int event_irq;
> +
> + void __iomem *mmio;
> + struct mrpc_regs __iomem *mmio_mrpc;
> + struct ntb_info_regs __iomem *mmio_ntb;
> + struct part_cfg_regs __iomem *mmio_part_cfg;
> +
> + /*
> + * The mrpc mutex must be held when accessing the other
> + * mrpc fields
> + */
> + struct mutex mrpc_mutex;
> + struct list_head mrpc_queue;
> + int mrpc_busy;
> + struct work_struct mrpc_work;
> + struct delayed_work mrpc_timeout;
> +};
> +
> +#define stdev_pdev(stdev) ((stdev)->pdev)
> +#define stdev_pdev_dev(stdev) (&stdev_pdev(stdev)->dev)
> +#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
> +#define stdev_dev(stdev) ((stdev)->dev)
Ick, just open code these please. That's a huge hint your use of the
driver model is not correct :)
thanks,
greg k-h
next prev parent reply other threads:[~2016-12-18 7:51 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-17 17:09 [RFC 0/1] New PCI Switch Management Driver Logan Gunthorpe
2016-12-17 17:09 ` Logan Gunthorpe
2016-12-17 17:09 ` [RFC 1/1] MicroSemi Switchtec management interface driver Logan Gunthorpe
2016-12-17 17:09 ` Logan Gunthorpe
2016-12-18 7:51 ` Greg Kroah-Hartman [this message]
2016-12-18 7:51 ` Greg Kroah-Hartman
2016-12-18 17:20 ` Logan Gunthorpe
2016-12-18 17:20 ` Logan Gunthorpe
2016-12-19 6:35 ` Greg Kroah-Hartman
2016-12-19 6:35 ` Greg Kroah-Hartman
2016-12-19 17:02 ` Keith Busch
2016-12-19 17:02 ` Keith Busch
2016-12-19 17:07 ` Logan Gunthorpe
2016-12-19 17:07 ` Logan Gunthorpe
2016-12-19 16:09 ` [RFC 0/1] New PCI Switch Management Driver Myron Stowe
2016-12-19 16:09 ` Myron Stowe
2016-12-19 17:06 ` Logan Gunthorpe
2016-12-19 17:06 ` Logan Gunthorpe
2016-12-19 17:29 ` Keith Busch
2016-12-19 17:29 ` Keith Busch
2016-12-19 17:26 ` Logan Gunthorpe
2016-12-19 17:26 ` Logan Gunthorpe
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=20161218075141.GE29850@kroah.com \
--to=gregkh@linuxfoundation.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.