All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Rafał Miłecki" <zajec5@gmail.com>
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC][PATCH V3] axi: add AXI bus driver
Date: Tue, 12 Apr 2011 13:40:36 +0200	[thread overview]
Message-ID: <BANLkTikR2AwBdORYUdTCcAOPeMkKN7azQQ@mail.gmail.com> (raw)
In-Reply-To: <BANLkTi=cG3U8yXaQ6PusmNaWcr+AWv3e_w@mail.gmail.com>

2011/4/12 Rafa? Mi?ecki <zajec5@gmail.com>:
> 2011/4/12 Greg KH <greg@kroah.com>:
>> On Tue, Apr 12, 2011 at 12:45:33AM +0200, Rafa? Mi?ecki wrote:
>>> 2011/4/12 Greg KH <greg@kroah.com>:
>>> > Then in your release function, free the struct axi_device. ?It's that
>>> > simple. ?To try to free it before then would be wrong and cause
>>> > problems.
>>>
>>> This is because it is defined as:
>>> struct axi_device cores[AXI_MAX_NR_CORES];
>>
>> No way, seriously?
>>
>> You can't do that, no static struct devices please. ?Make these dynamic
>> and everything will be fine. ?The -mm tree used to have a huge warning
>> if you ever tried to register a statically allocated struct, but that
>> didn't really work out, but would have saved you a lot of time here,
>> sorry.
>>
>> So dynamically allocate the structures and you will be fine.
>
> Well, I saw that along kernel, I had no idea there is anything wrong
> about this. It seems more ppl do not know about this:
> struct radeon_ib ? ? ? ?ibs[RADEON_IB_POOL_SIZE];
> struct radeon_pm_clock_info clock_info[8];
> struct radeon_pm_profile profiles[PM_PROFILE_MAX];
> struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
> struct radeon_i2c_chan *i2c_bus[RADEON_MAX_I2C_BUS];
>
> struct b43_key key[B43_NR_GROUP_KEYS * 2 + B43_NR_PAIRWISE_KEYS];
>
> struct ssb_device devices[SSB_MAX_NR_CORES];
> I guess I could fine more examples by simple grepping .h files.
>
> Is there some guide around with things like this we should avoid?
> checkpatch does no catch this, so maybe just some manual? Could you
> point me to it?

Greg, my:
struct ssb_device devices[SSB_MAX_NR_CORES];
is part of "struct axi_bus", which we allocate dynamically anyway:

struct axi_bus *bus;
bus = kzalloc(sizeof(*bus), GFP_KERNEL);
if (!bus)
	goto out;

So do we really need to dynamically alloc main structure and
separately every of it's array of structs? Does it really make sense?
Please point me to some place where I can read more about this. Some
tips about coding style for such things, cases.

-- 
Rafa?

WARNING: multiple messages have this Message-ID (diff)
From: "Rafał Miłecki" <zajec5@gmail.com>
To: Greg KH <greg@kroah.com>
Cc: linux-wireless@vger.kernel.org,
	"John W. Linville" <linville@tuxdriver.com>,
	"George Kashperko" <george@znau.edu.ua>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Russell King" <rmk@arm.linux.org.uk>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	b43-dev@lists.infradead.org, "Michael Büsch" <mb@bu3sch.de>,
	linuxdriverproject <devel@linuxdriverproject.org>,
	"Andy Botting" <andy@andybotting.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"Larry Finger" <Larry.Finger@lwfinger.net>
Subject: Re: [RFC][PATCH V3] axi: add AXI bus driver
Date: Tue, 12 Apr 2011 13:40:36 +0200	[thread overview]
Message-ID: <BANLkTikR2AwBdORYUdTCcAOPeMkKN7azQQ@mail.gmail.com> (raw)
In-Reply-To: <BANLkTi=cG3U8yXaQ6PusmNaWcr+AWv3e_w@mail.gmail.com>

2011/4/12 Rafał Miłecki <zajec5@gmail.com>:
> 2011/4/12 Greg KH <greg@kroah.com>:
>> On Tue, Apr 12, 2011 at 12:45:33AM +0200, Rafał Miłecki wrote:
>>> 2011/4/12 Greg KH <greg@kroah.com>:
>>> > Then in your release function, free the struct axi_device.  It's that
>>> > simple.  To try to free it before then would be wrong and cause
>>> > problems.
>>>
>>> This is because it is defined as:
>>> struct axi_device cores[AXI_MAX_NR_CORES];
>>
>> No way, seriously?
>>
>> You can't do that, no static struct devices please.  Make these dynamic
>> and everything will be fine.  The -mm tree used to have a huge warning
>> if you ever tried to register a statically allocated struct, but that
>> didn't really work out, but would have saved you a lot of time here,
>> sorry.
>>
>> So dynamically allocate the structures and you will be fine.
>
> Well, I saw that along kernel, I had no idea there is anything wrong
> about this. It seems more ppl do not know about this:
> struct radeon_ib        ibs[RADEON_IB_POOL_SIZE];
> struct radeon_pm_clock_info clock_info[8];
> struct radeon_pm_profile profiles[PM_PROFILE_MAX];
> struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
> struct radeon_i2c_chan *i2c_bus[RADEON_MAX_I2C_BUS];
>
> struct b43_key key[B43_NR_GROUP_KEYS * 2 + B43_NR_PAIRWISE_KEYS];
>
> struct ssb_device devices[SSB_MAX_NR_CORES];
> I guess I could fine more examples by simple grepping .h files.
>
> Is there some guide around with things like this we should avoid?
> checkpatch does no catch this, so maybe just some manual? Could you
> point me to it?

Greg, my:
struct ssb_device devices[SSB_MAX_NR_CORES];
is part of "struct axi_bus", which we allocate dynamically anyway:

struct axi_bus *bus;
bus = kzalloc(sizeof(*bus), GFP_KERNEL);
if (!bus)
	goto out;

So do we really need to dynamically alloc main structure and
separately every of it's array of structs? Does it really make sense?
Please point me to some place where I can read more about this. Some
tips about coding style for such things, cases.

-- 
Rafał

WARNING: multiple messages have this Message-ID (diff)
From: zajec5@gmail.com (Rafał Miłecki)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC][PATCH V3] axi: add AXI bus driver
Date: Tue, 12 Apr 2011 13:40:36 +0200	[thread overview]
Message-ID: <BANLkTikR2AwBdORYUdTCcAOPeMkKN7azQQ@mail.gmail.com> (raw)
In-Reply-To: <BANLkTi=cG3U8yXaQ6PusmNaWcr+AWv3e_w@mail.gmail.com>

2011/4/12 Rafa? Mi?ecki <zajec5@gmail.com>:
> 2011/4/12 Greg KH <greg@kroah.com>:
>> On Tue, Apr 12, 2011 at 12:45:33AM +0200, Rafa? Mi?ecki wrote:
>>> 2011/4/12 Greg KH <greg@kroah.com>:
>>> > Then in your release function, free the struct axi_device. ?It's that
>>> > simple. ?To try to free it before then would be wrong and cause
>>> > problems.
>>>
>>> This is because it is defined as:
>>> struct axi_device cores[AXI_MAX_NR_CORES];
>>
>> No way, seriously?
>>
>> You can't do that, no static struct devices please. ?Make these dynamic
>> and everything will be fine. ?The -mm tree used to have a huge warning
>> if you ever tried to register a statically allocated struct, but that
>> didn't really work out, but would have saved you a lot of time here,
>> sorry.
>>
>> So dynamically allocate the structures and you will be fine.
>
> Well, I saw that along kernel, I had no idea there is anything wrong
> about this. It seems more ppl do not know about this:
> struct radeon_ib ? ? ? ?ibs[RADEON_IB_POOL_SIZE];
> struct radeon_pm_clock_info clock_info[8];
> struct radeon_pm_profile profiles[PM_PROFILE_MAX];
> struct radeon_surface_reg surface_regs[RADEON_GEM_MAX_SURFACES];
> struct radeon_i2c_chan *i2c_bus[RADEON_MAX_I2C_BUS];
>
> struct b43_key key[B43_NR_GROUP_KEYS * 2 + B43_NR_PAIRWISE_KEYS];
>
> struct ssb_device devices[SSB_MAX_NR_CORES];
> I guess I could fine more examples by simple grepping .h files.
>
> Is there some guide around with things like this we should avoid?
> checkpatch does no catch this, so maybe just some manual? Could you
> point me to it?

Greg, my:
struct ssb_device devices[SSB_MAX_NR_CORES];
is part of "struct axi_bus", which we allocate dynamically anyway:

struct axi_bus *bus;
bus = kzalloc(sizeof(*bus), GFP_KERNEL);
if (!bus)
	goto out;

So do we really need to dynamically alloc main structure and
separately every of it's array of structs? Does it really make sense?
Please point me to some place where I can read more about this. Some
tips about coding style for such things, cases.

-- 
Rafa?

  reply	other threads:[~2011-04-12 11:40 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-11 21:25 [RFC][PATCH V3] axi: add AXI bus driver Rafał Miłecki
2011-07-08 18:29 ` Rafał Miłecki
2011-04-11 21:25 ` Rafał Miłecki
2011-04-11 21:06 ` Greg KH
2011-04-11 21:06   ` Greg KH
2011-04-11 21:19   ` Rafał Miłecki
2011-04-11 21:19     ` Rafał Miłecki
2011-04-11 21:19     ` Rafał Miłecki
2011-04-11 21:25     ` Greg KH
2011-04-11 21:25       ` Greg KH
2011-04-11 21:36       ` Rafał Miłecki
2011-04-11 21:36         ` Rafał Miłecki
2011-04-11 21:36         ` Rafał Miłecki
2011-04-11 21:56         ` Greg KH
2011-04-11 21:56           ` Greg KH
2011-04-11 22:12           ` Rafał Miłecki
2011-04-11 22:12             ` Rafał Miłecki
2011-04-11 22:12             ` Rafał Miłecki
2011-04-11 22:36             ` Greg KH
2011-04-11 22:36               ` Greg KH
2011-04-11 22:45               ` Rafał Miłecki
2011-04-11 22:45                 ` Rafał Miłecki
2011-04-11 22:45                 ` Rafał Miłecki
2011-04-11 23:35                 ` Greg KH
2011-04-11 23:35                   ` Greg KH
2011-04-12  5:41                   ` Rafał Miłecki
2011-04-12  5:41                     ` Rafał Miłecki
2011-04-12  5:41                     ` Rafał Miłecki
2011-04-12 11:40                     ` Rafał Miłecki [this message]
2011-04-12 11:40                       ` Rafał Miłecki
2011-04-12 11:40                       ` Rafał Miłecki
2011-04-12 15:16                       ` Greg KH
2011-04-12 15:16                         ` Greg KH
2011-04-12 15:14                     ` Greg KH
2011-04-12 15:14                       ` Greg KH
2011-04-12 15:25                       ` Rafał Miłecki
2011-04-12 15:25                         ` Rafał Miłecki
2011-04-12 15:25                         ` Rafał Miłecki
2011-04-11 21:07 ` Greg KH
2011-04-11 21:07   ` Greg KH
2011-04-11 23:44 ` Joe Perches
2011-04-11 23:44   ` Joe Perches
2011-04-12  5:52   ` Rafał Miłecki
2011-04-12  5:52     ` Rafał Miłecki
2011-04-12  5:52     ` Rafał Miłecki

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=BANLkTikR2AwBdORYUdTCcAOPeMkKN7azQQ@mail.gmail.com \
    --to=zajec5@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.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.