netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Krzysztof Halasa <khc@pm.waw.pl>
To: Jonas Gorski <jogo@openwrt.org>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	"David S. Miller" <davem@davemloft.net>,
	Russell King <linux@arm.linux.org.uk>,
	Imre Kaloz <kaloz@openwrt.org>
Subject: Re: [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices
Date: Sun, 21 Jul 2013 16:45:45 +0200	[thread overview]
Message-ID: <m3ip04ezty.fsf@intrepid.localdomain> (raw)
In-Reply-To: <1374315339-31469-2-git-send-email-jogo@openwrt.org> (Jonas Gorski's message of "Sat, 20 Jul 2013 12:15:38 +0200")

Jonas Gorski <jogo@openwrt.org> writes:

> ARM requires the cohorent_dma_mask set, so set it for the platform
> devices so that the ethernet driver has access to it.

I recognize the need to fix this issue and I appreciate your efforts,
but... I think this patch tries to make the driver functional again at
all costs and this a very bad idea. The IXP4xx Ethernet MACs are not
normal platform devices, they are in fact built-in CPU resources. The
platform device structs are only used to set parameters. What the patch
does is unneeded and IMHO harmful code duplication. It makes completely
no sense to set DMA masks in code for individual platforms as it's not
something platforms can decide, or *even should know of*. It's simply
a CPU attribute, a value that is shared by all IXP4xx CPUs and thus all
platforms and systems using it.

This is against the "line of code" count rules (or "rules").

Also the dev->dev.parent is IMHO a bad idea. The queue numbers and MAC
addresses are in no way "parents" of Ethernet controllers, and even if
they somehow were, I find it rather hard to believe they can have DMA
masks.

I think the previous patch (which sets the masks in one place, in
Ethernet driver code) was better, though not perfect.

My fault is I haven't fixed it yet. Will try to invent something.
-- 
Krzysztof Halasa

  reply	other threads:[~2013-07-21 14:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-20 10:15 [PATCH 0/2] net: fix ixp4xx_eth Jonas Gorski
2013-07-20 10:15 ` [PATCH 1/2] arm: ixp4xx: set cohorent_dma_mask for ethernet platform devices Jonas Gorski
2013-07-21 14:45   ` Krzysztof Halasa [this message]
2013-07-21 16:40     ` Ben Hutchings
2013-07-22 14:49       ` Krzysztof Halasa
2013-07-20 10:15 ` [PATCH 2/2] net: ixp4xx_eth: use parent device for dma allocations Jonas Gorski

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=m3ip04ezty.fsf@intrepid.localdomain \
    --to=khc@pm.waw.pl \
    --cc=davem@davemloft.net \
    --cc=jogo@openwrt.org \
    --cc=kaloz@openwrt.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=netdev@vger.kernel.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 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).