All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code
@ 2005-05-13  8:03 Rolf Eike Beer
  2005-05-17 15:41 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
                   ` (22 more replies)
  0 siblings, 23 replies; 24+ messages in thread
From: Rolf Eike Beer @ 2005-05-13  8:03 UTC (permalink / raw)
  To: linux-ia64

[-- Attachment #1: Type: text/plain, Size: 6497 bytes --]

Prarit Bhargava wrote:
> This patch is the SGI hotplug driver and additional changes required for
> the driver.  These modifications include changes to the SN io_init.c code
> for memory management, the inclusion of new SAL calls to enable and disable
> PCI slots, and a hotplug-style driver.
>
> Signed-off-by: Prarit Bhargava <prarit@sgi.com>

> Index: drivers/pci/hotplug/sgi_hotplug.c
> ===================================================================
> --- /dev/null  (tree:94b3c225a19fb0e688da58fa7c9239db0a743e9c)
> +++
> d42322aa13214a5d099019ff0406dfb328960b98/drivers/pci/hotplug/sgi_hotplug.c 
> (mode:100644 sha1:323041fd41dc19c20ce88e4ae370272cbe09ea97)

> +static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot,
> +				    struct pci_bus *pci_bus, int device)
> +{
> +	struct pcibus_info *pcibus_info;
> +	struct slot *slot;
> +
> +	pcibus_info = SN_PCIBUS_BUSSOFT_INFO(pci_bus);
> +
> +	bss_hotplug_slot->private = kcalloc(1, sizeof(struct slot),
> +					    GFP_KERNEL);
> +	if (!bss_hotplug_slot->private)
> +		return -ENOMEM;
> +	slot = (struct slot *)bss_hotplug_slot->private;

Please remove this cast. Since private is a void* there is never a cast needed 
to or from this.

> +	bss_hotplug_slot->name = kmalloc(33, GFP_KERNEL);

I would feel better if you would use a #define here like the other drivers do. 
They normally name is SLOT_NAME_SIZE or something similar.

> +	sprintf(bss_hotplug_slot->name, "module_%c%c%c%c%.2d_b_%d_s_%d",
> +		'0'+RACK_GET_CLASS(MODULE_GET_RACK(pcibus_info->pbi_moduleid)),
> +		'0'+RACK_GET_GROUP(MODULE_GET_RACK(pcibus_info->pbi_moduleid)),
> +		'0'+RACK_GET_NUM(MODULE_GET_RACK(pcibus_info->pbi_moduleid)),
> +		MODULE_GET_BTCHAR(pcibus_info->pbi_moduleid),
> +		MODULE_GET_BPOS(pcibus_info->pbi_moduleid),
> +		((int)pcibus_info->pbi_buscommon.bs_persist_busnum) & 0xf,
> +		device + 1);

*eek* Sorry, but that looks really ugly. Wouldn't it be enough do name it like 
the device that would be in slot, something like DOMAIN_BUS_SLOT and maybe 
one extra number?

> +static struct hotplug_slot * sn_hp_destroy(void)

Please remove the space before the function name

> +static u8 sn_power_status_get(struct hotplug_slot *bss_hotplug_slot)

Why not do this few instructions in get_power_status() directly? Or at least 
mark it inline, you only use it once and it does not really consume much 
stack space.

> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

Remove cast.

> +static void sn_slot_mark_enable(struct hotplug_slot *bss_hotplug_slot,
> +				int device_num)

Add inline or inline it by hand.

> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

Cast.

> +static void sn_slot_mark_disable(struct hotplug_slot *bss_hotplug_slot,
> +				 int device_num)
> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

The same.

> +static int sn_slot_enable(struct hotplug_slot *bss_hotplug_slot,
> +			  int device_num)
> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

Cast.

> +	if (rc == PCI_SLOT_ALREADY_UP) {
> +		dev_dbg(slot->pci_bus->self, "is already active\n");
> +		return -EPERM;
> +	}

IIRC most other drivers handle this case as success. Greg, your opinion?

> +	if (rc) {
> +		dev_dbg(slot->pci_bus->self,
> +			"insert failed with error %d sub-error %d\n",
> +			rc, resp.resp_sub_errno);
> +		return -EIO;
> +	}
> +
> +	sn_slot_mark_enable(bss_hotplug_slot, device_num);
> +
> +	return 0;
> +}

Replacing the last function call with 4 lines of instructions won't hurt I 
think. IMHO sn_slot_mark_enable() should be killed. Maybe you add a comment 
here to tell what's going on if you want to make it clear.

> +static int sn_slot_disable(struct hotplug_slot *bss_hotplug_slot,
> +			   int device_num, int action)
> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

Cast.

> +	if (action == PCI_REQ_SLOT_ELIGIBLE && rc == PCI_SLOT_ALREADY_DOWN) {

I would feel better if you add extra braces around the tests, but I'm probably 
just a bit paranoid about this things. Greg?

> +		dev_dbg(slot->pci_bus->self, "Slot %s already inactive\n");
> +		return -ENODEV;
> +	}

Again this might better be a success.

> +	if (action == PCI_REQ_SLOT_DISABLE && rc) {
> +		dev_dbg(slot->pci_bus->self,"remove failed rc = %d\n", rc);
> +		return rc;
> +	}
> +
> +	return rc;
> +}

One return should be enough for everyone.

> +static int enable_slot(struct hotplug_slot *bss_hotplug_slot)
> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

Cast.

> +	num_funcs = pci_scan_slot(slot->pci_bus, PCI_DEVFN(slot->device_num+1,

Add spaces before and after '+'. I don't feel good with this "+1" at all, this 
is some kind of strange.

> +							   PCI_FUNC(0)));

Greg, what do you think. Isn't just 0 better here?

Please wrap the line one level higher and keep the second argument as one, so 
add the break behind "slot->pci_bus,".

> +static int disable_slot(struct hotplug_slot *bss_hotplug_slot)
> +{
> +	struct slot *slot = (struct slot *)bss_hotplug_slot->private;

Cast.

> +static int get_power_status(struct hotplug_slot *bss_hotplug_slot, u8
> *value) +{
> +	down(&sn_hotplug_sem);
> +	*value = sn_power_status_get(bss_hotplug_slot);
> +	up(&sn_hotplug_sem);
> +	return 0;
> +}

sn_power_status_get() should be easily be inlinable to here. This would allow 
to make the up()/down() protect even less instructions.

> +static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
> +{
> +	int device;
> +	struct hotplug_slot *bss_hotplug_slot;
> +	int rc = 0;
> +
> +	/*
> +	 * Currently only four devices are supported,
> +	 * in the future there maybe more -- up to 32.
> +	 */
> +
> +	for (device = 0; device < SN_MAX_HP_SLOTS ; device++) {
> +		if (sn_pci_slot_valid(pci_bus, device) != 1)
> +			continue;
> +
> +		bss_hotplug_slot = kcalloc(1,sizeof(struct hotplug_slot),
> +					   GFP_KERNEL);

Use "sizeof(*bss_hotplug_slot)" and add a space after the ','.

> +		bss_hotplug_slot->info =
> +			kcalloc(1,sizeof(struct hotplug_slot_info),
> +				GFP_KERNEL);

Same here.

> +static int sn_pci_hotplug_init(void)
> +{

> +		if (!rc)
> +			registered = 1;
> +		else {
> +			registered = 0;
> +			break;
> +		}

Please use {} for the if also to make it look saner.

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2005-05-22  1:13 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-05-13  8:03 [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Rolf Eike Beer
2005-05-17 15:41 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-17 20:17 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Greg KH
2005-05-18 11:33 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-18 11:42 ` Prarit Bhargava
2005-05-18 13:43 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Rolf Eike Beer
2005-05-18 13:46 ` Rolf Eike Beer
2005-05-18 13:57 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-18 14:06 ` Prarit Bhargava
2005-05-18 15:20 ` Prarit Bhargava
2005-05-18 16:23 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Greg KH
2005-05-18 16:41 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-18 16:52 ` Prarit Bhargava
2005-05-18 16:54 ` Prarit Bhargava
2005-05-18 17:06 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Greg KH
2005-05-19  7:56 ` Rolf Eike Beer
2005-05-19 13:05 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-19 15:13 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Greg KH
2005-05-20 12:11 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-20 18:26 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Greg KH
2005-05-20 23:47 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava
2005-05-21  0:24 ` Prarit Bhargava
2005-05-21  3:59 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code Greg KH
2005-05-22  1:13 ` [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver Prarit Bhargava

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.