Re: [Pcihpd-discuss] [PATCH 5/6]: hotplug/ia64: SN Hotplug Driver - SN Hotplug Driver code

[Rolf Eike Beer <eike-hotplug@sf-tec.de>]

[-- 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 --]